#72: Disable extraction in private browsing sessions. #168

Osmose · 2018-10-15T20:34:19Z

This handles part of #72, but not all of it. But it's a nice little reviewable unit, so why not?

I had to refactor how we do messaging between content scripts and the background script, but it was an overdue refactor anyway.

Osmose · 2018-10-15T22:45:14Z

Removing the review flag, I have some changes I wanna make.

Osmose · 2018-10-15T23:38:07Z

Okay, I think this is good now. Should handle all the cases we care about as blockers.

After landing this we'll probably want a followup issue for post-launch that handles letting users know that background price updates are disabled. Might need some UX help to get the UI right.

groovecoder · 2018-10-16T20:17:45Z

src/background/privacy.js

+  // Cookie blocking
+  if (browser.privacy.websites.cookieConfig) {
+    const {behavior} = (await browser.privacy.websites.cookieConfig.get({})).value;
+    if (['reject_all', 'reject_third_party', 'reject_trackers'].includes(behavior)) {


Suggested change

if (['reject_all', 'reject_third_party', 'reject_trackers'].includes(behavior)) {

if (!['allow_all', 'allow_visited'].includes(behavior)) {

This may help future-proof the value set in case, e.g., reject_trackers changes, or another cookie behavior is added.

groovecoder

This looks good. Made a minor change suggestion (whoa - new GitHub feature!) that could help future-proof the arePrivacyControlsActive logic. But r+ either way.

Osmose · 2018-10-16T20:38:31Z

Updated; turns out content scripts can tell whether they're in an incognito context, which removes the need for refactoring the message system; I'll be putting that in a separate PR to simplify this one.

biancadanforth

Thanks Osmose for the PR! Great idea to make use of the browser.privacy and browser.extension WE APIs to avoid adding more experimental APIs to handle these privacy preferences.

This works as expected in every case except for one per the table we discussed earlier today: background updates are still happening in private windows (see review comments). Unfortunately, I couldn't find a nice solution for this, though maybe you'll have better luck.

biancadanforth · 2018-10-17T02:37:18Z

src/browser_action/components/BrowserActionApp.jsx

@@ -16,6 +16,8 @@ import {removeMarkedProducts} from 'commerce/state/products';

 import 'commerce/browser_action/components/BrowserActionApp.css';

+import {recordEvent} from 'commerce/background/telemetry';


Probably want to remove this for the PR.

biancadanforth · 2018-10-17T02:37:48Z

src/browser_action/components/BrowserActionApp.jsx

@@ -55,6 +57,7 @@ export default class BrowserActionApp extends React.Component {
  }

  componentDidMount() {
+    recordEvent();


Same goes for here.

YOU SAW NOTHING

biancadanforth · 2018-10-17T02:46:04Z

src/privacy.js

+ * @return {boolean}
+ */
+export async function shouldExtract() {
+  return !browser.extension.inIncognitoContext;


browser.extension.inIncognitoContext incorrectly returns false for background extraction when a private window is active. This happens both in the case that only a single private window is open, and when both a private and normal window are open with the private window active.

This means we're getting background extraction (price polling) in Private Windows currently. I tried an alternative (browser.windows.getCurrent), but unfortunately, browser.windows is undefined in the background iframe content scripts.

I'm not sure what other options would be except to:

Store the incognito status of the current window in state, or

Message pass from the content script to the background script to obtain this information through the background script.

Reading the docs on browser.extension.inIncognitoContext:

Boolean value, true for content scripts running inside private browsing tabs and for extension pages running inside a private browsing process.

Neither of these is the case for a background extraction.

Yeah, this is why I keep on asking about what we mean when we say "no background updates for private browsing".

Background updates don't happen in the currently active window. They happen in the background. The user can have private and non-private windows open. I don't understand what "no backgroud updates for private browsing" because private browsing is not a thing that can apply to the background script. It's never in any "private browsing" mode. I'm probably being a bit pedantic here.

So far only you have hinted at a phrase that actually makes sense to me: Disable background updates as long as a private window is the active window. However, I don't think that actually makes sense. If a user briefly switches to a non-private browser window and we start updating prices, but then the user switches back to the private window, are we to halt the existing updates immediately? If they briefly switch to private browsing mode when the update timer ticks, and then go back to non-private mode, do they miss updates for the next X minutes? What if they have a private browsing window open, but it's in the background?

IMO private browsing mode is a signal that specific tabs are private, and we should (as this PR does) disable extraction and telemetry for those tabs, but otherwise function as normal with regards to background price updates, as private browsing doesn't express a user intent to disable all activity. There's still plenty of other privacy signals that disable background updates, like DNT.

@groovecoder @biancadanforth What do you think?

... as private browsing doesn't express a user intent to disable all activity

I'm not sure we really can say what the user intent is and whether it extends to include allowing background price updates. In a previous Tracking Protection study, one of the key attitudinal findings was that some users didn't really understand what Tracking Protection was.

I can say that based on the SUMO page for Private Browsing, Tracking Protection is enabled by default. Since we disable background price updates when Tracking Protection is enabled, allowing background price updates to continue in Private Browsing windows when Tracking Protection is also supposed to be enabled seems contradictory.

(Note: @groovecoder I am seeing Content Blocking and Tracking Protection equated in SUMO, and Content Blocking on by default in normal, non-private windows in Nightly, yet the privacy.trackingprotection.enabled pref is false at the same time -- what is going on here? If Content Blocking is Tracking Protection, and it's default on for non-private windows, I don't see how we can impose the behavior of no background price updates when it's on.)

Anyway, I agree that the behavior is more nuanced than the table suggests for Private Browsing windows. My proposal would be that we check once each time before initiating background price updates in the same manner as we do for the other privacy contexts like DNT. If we want to consider adding change listeners, that can be a separate issue. Unfortunately, this check will not be as simple as the others, since we can't directly ascertain whether the currently focused window is incognito from the background extraction content script.

This is my opinion of course; I ultimately defer to @groovecoder as to whether it's okay to leave as-is.

@Osmose reasoning makes sense - we should disable extraction & telemetry for PBM tabs.

@biancadanforth reason also makes sense - we could disable background updates while a PBM tab is the active tab.

If that's overly complicated to implement, maybe disable background updates if there are only PBM windows/tabs open. (I.e., for users who select "Always use Private Browsing" or manually close all regular browsing windows) But that sounds like even more if checks 😐

This PR is certainly an improvement and I hate letting code bit-rot in long-winded PRs, so I'm 👍 on this logic, with a more nuanced PBM implementation as a non-blocking follow-up enhancement.

Filed #177 for a potential follow-up.

biancadanforth · 2018-10-17T02:46:48Z

src/privacy.js

+  }
+
+  // Content scripts in private browsing windows don't collect telemetry.
+  if (browser.extension.inIncognitoContext) {


As mentioned above, we can't rely on this exclusively to tell us if the current window is a private window.

biancadanforth · 2018-10-17T02:58:39Z

src/privacy.js

+  }
+
+  // Do Not Track
+  if (navigator.doNotTrack === '1') {


This seems to map with the privacy.donottrackheader.enabled preference! Nice job not having to add more experimental APIs to handle this.

biancadanforth · 2018-10-17T03:04:20Z

src/privacy.js

+  // Cookie blocking
+  if (browser.privacy.websites.cookieConfig) {
+    const {behavior} = (await browser.privacy.websites.cookieConfig.get({})).value;
+    if (!['allow_all', 'allow_visited'].includes(behavior)) {


This also maps to the network.cookie.cookieBehavior int preference.

0: 'allow_all'

1: 'reject_third_party'

2: 'reject_all'

3: 'allow_visited'

4: 'reject_trackers'

In #72 , @groovecoder only specified turning off background extraction (price polling) for the 4 setting of the preference. As written, this currently blocks both telemetry and price polling for the values of 1 and 2. I guess those settings are even more restrictive and therefore encompass a setting of 4?

It blocks both for any values that aren't 0 or 3, which includes 4. It's pretty much "If the user is specifically blocking any kind of cookies, they probably desire the reduced-tracking mode of the add-on".

I missed that telemetry wasn't included in things that are disabled for this particular preference. I can update to that effect.

The table in #72 (comment) that we approved has Telemetry collection set to false when cookies are being rejected. This matches the implementation in this PR, right? It's a change from his comment but matches what we agreed to later on.

Yes, when I made that table, I had not yet reviewed this PR, so I didn't know specifically how a setting of 4 was different than any other setting. What you've done makes sense to me and is consistent with the table we agreed on.

Osmose · 2018-10-17T17:26:45Z

Updated with removed testing lines and a small refactor to improve the readability of the privacy module.

biancadanforth

I left some replies to your comments in line. As I mention there, I defer to @groovecoder as to whether background price updates are okay when the user is in a private window. My opinion is that we should disable them, though I acknowledge the implementation for that is not straightforward.

Everything else seems great! Thanks for cleaning up privacy.js; it's very clear now where you are checking what.

I am dismissing my original review over approving changes, since the remaining change I believe should be made is ultimately a call best made by groovecoder.

groovecoder · 2018-10-18T14:49:14Z

src/privacy.js

+ * @return {boolean}
+ */
+export async function shouldExtract() {
+  return !browser.extension.inIncognitoContext;


@Osmose reasoning makes sense - we should disable extraction & telemetry for PBM tabs.

@biancadanforth reason also makes sense - we could disable background updates while a PBM tab is the active tab.

If that's overly complicated to implement, maybe disable background updates if there are only PBM windows/tabs open. (I.e., for users who select "Always use Private Browsing" or manually close all regular browsing windows) But that sounds like even more if checks 😐

This PR is certainly an improvement and I hate letting code bit-rot in long-winded PRs, so I'm 👍 on this logic, with a more nuanced PBM implementation as a non-blocking follow-up enhancement.

groovecoder · 2018-10-18T14:55:19Z

Nice refactoring and updates, too - very read-able and straight-forward.

…tings.

Osmose · 2018-10-18T22:31:03Z

Thanks ya'll!

Osmose requested review from biancadanforth and removed request for biancadanforth October 15, 2018 20:34

Osmose force-pushed the privacy-controls branch 2 times, most recently from 138a17c to 565941f Compare October 15, 2018 23:36

Osmose requested review from groovecoder and biancadanforth October 15, 2018 23:37

groovecoder reviewed Oct 16, 2018

View reviewed changes

groovecoder approved these changes Oct 16, 2018

View reviewed changes

Osmose force-pushed the privacy-controls branch 2 times, most recently from 569256f to d602fe1 Compare October 16, 2018 20:37

Osmose mentioned this pull request Oct 16, 2018

Centralize message handling to allow for multiple async listeners. #170

Merged

biancadanforth previously requested changes Oct 17, 2018

View reviewed changes

Osmose force-pushed the privacy-controls branch from d602fe1 to 7d9d324 Compare October 17, 2018 17:25

biancadanforth reviewed Oct 17, 2018

View reviewed changes

groovecoder approved these changes Oct 18, 2018

View reviewed changes

groovecoder mentioned this pull request Oct 18, 2018

Non-blocking: consider nuanced background price polling when PBM tabs exist #177

Closed

Michael Kelly added 3 commits October 18, 2018 15:17

Fix #72: Disable extraction, telemetry, price updates for privacy set…

07e0673

…tings.

Refactor privacy checks for easier readability.

9e19a9d

Remove leftover testing lines.

42a19c7

Osmose force-pushed the privacy-controls branch from 57c5b67 to 42a19c7 Compare October 18, 2018 22:17

Osmose merged commit 402bc22 into master Oct 18, 2018

Osmose deleted the privacy-controls branch October 18, 2018 22:30

biancadanforth mentioned this pull request Oct 19, 2018

Fix #156: Add extension UI probes #169

Merged

biancadanforth mentioned this pull request Oct 19, 2018

Disable extraction and telemetry in PBM, with DNT and/or Tracking Protection on #72

Closed

1 task

This was referenced Nov 14, 2018

Collect category 1-2 data and enable background price updates from consenting users in privacy-sensitive contexts #186

Closed

Fix #177: Disable price polling in Private Browsing windows #276

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#72: Disable extraction in private browsing sessions. #168

#72: Disable extraction in private browsing sessions. #168

Osmose commented Oct 15, 2018

Osmose commented Oct 15, 2018

Osmose commented Oct 15, 2018

groovecoder Oct 16, 2018

groovecoder Oct 16, 2018

groovecoder left a comment

Osmose commented Oct 16, 2018

biancadanforth left a comment

biancadanforth Oct 17, 2018

biancadanforth Oct 17, 2018

Osmose Oct 17, 2018

biancadanforth Oct 17, 2018

biancadanforth Oct 17, 2018

Osmose Oct 17, 2018

biancadanforth Oct 17, 2018 •

edited

Loading

groovecoder Oct 18, 2018

groovecoder Oct 18, 2018

biancadanforth Oct 17, 2018

biancadanforth Oct 17, 2018

biancadanforth Oct 17, 2018

Osmose Oct 17, 2018

Osmose Oct 17, 2018 •

edited

Loading

biancadanforth Oct 17, 2018

Osmose commented Oct 17, 2018

biancadanforth left a comment

groovecoder Oct 18, 2018

groovecoder commented Oct 18, 2018

Osmose commented Oct 18, 2018

	if (['reject_all', 'reject_third_party', 'reject_trackers'].includes(behavior)) {
	if (!['allow_all', 'allow_visited'].includes(behavior)) {

		@@ -16,6 +16,8 @@ import {removeMarkedProducts} from 'commerce/state/products';

		import 'commerce/browser_action/components/BrowserActionApp.css';

		import {recordEvent} from 'commerce/background/telemetry';

#72: Disable extraction in private browsing sessions. #168

#72: Disable extraction in private browsing sessions. #168

Conversation

Osmose commented Oct 15, 2018

Osmose commented Oct 15, 2018

Osmose commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder left a comment

Choose a reason for hiding this comment

Osmose commented Oct 16, 2018

biancadanforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Osmose Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Osmose commented Oct 17, 2018

biancadanforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder commented Oct 18, 2018

Osmose commented Oct 18, 2018

biancadanforth Oct 17, 2018 •

edited

Loading

Osmose Oct 17, 2018 •

edited

Loading