added option to disable DNT policy checking #1391

Ghoughpteighbteau · 2017-05-22T23:53:26Z

adds an option to disable DNT policy checks. When this option is disabled, domains fall back to heuristics and privacy badger stops asking the EFF for updated policy hashes.

I couldn't think of a better way to test these settings, and I couldn't find any of the other settings being tested, so I didn't have anything to emulate.

Let me know what needs changed here!

ghostwords · 2017-05-23T00:11:26Z

Cool, thank you, I'll go over this tomorrow. Looks like some tests are failing; let me know if I can help getting them to run locally.

For our reference, here is where the toggle happened in the legacy Firefox repo: EFForg/privacybadgerfirefox-legacy#236

ghostwords · 2017-05-23T00:17:15Z

One bit of feedback I have now is that in order for Privacy Badger to continue to function as a force for effecting positive change on the Web, Do Not Track policy checking must be enabled by default.

Ghoughpteighbteau · 2017-05-23T00:31:18Z

erg. I'll have to get selenium running to see what I broke. darn!

BTW it's enabled by default!

edit: ARG I'M DUMB. it was not enabled by default. All the tests that expected DNT to work broke, obviously. 😅

ghostwords · 2017-05-23T19:55:06Z

src/skin/options.html

+          <input type="checkbox" id="check_dnt_policy_checkbox">
+          <span class="i18n_options_dnt_policy_setting"></span>
+        </label>
+        <a target="_blank" href="https://www.eff.org/dnt-policy"/><span class="i18n_options_dnt_policy_learn_more"></span></a>


Typo: unwanted "/" in opening "a" tag.

ghostwords · 2017-05-23T20:02:05Z

src/tests/tests/background.js

+
+    assert.expect(NUM_TESTS);
+
+    badger.isCheckingDNTPolicyEnabled = () => false;


Should just change (and restore) the setting inside this test. Since we don't reuse this override, no need for "before" and "after" bits, I think.

I've been thinking about this one. I think this needs to be more generalized for the state of the local storage in all unit tests. The thing that happens in the storage.js test should happen in all tests. All unit tests should save the local storage, replace with a standard one, and per test change it depending on the thing they are looking at. I think this wasn't done before because I think all the unit tests just avoided messing with anything that changed with settings.

Agree that the fact that unit tests can modify local settings when run against your local Badger is a bug. It would be good to get it fixed, but it's not a blocker for this pull request.

K. in the mean time I'll move the set and unset bits to this test, and mark it with a TODO

OK now I think we are running into an annoying async test issue. I'm seeing other DNT-related tests break I think because we don't restore isCheckingDNTPolicyEnabled at exactly the right time: restoring at the end of the test isn't guaranteed to work because the test is async.

Ah, never mind, I somehow modified my local settings messing with the tests. Yeah, would be great to avoid this issue entirely.

ghostwords · 2017-05-23T20:21:51Z

src/tests/tests/storage.js

@@ -164,6 +164,8 @@
  });

  QUnit.test("DNT does not cascade", (assert) => {
+    let settings_map = storage.getBadgerStorageObject('settings_map');
+    settings_map.setItem("checkForDNTPolicy", true);


These two lines should be removed as unnecessary, right?

nope. nasty catch there, if your local version is false then this gets messed up. If I hit the unit tests of the version of privacy badger currently installed on my browser, my settings would change the context of the test, and in the case of this feature, alter the outcome of several tests :S

Good point, but that's a bug with our testing setup. Let's fix or open a ticket to fix this unit tests bug instead of resetting settings in every test that uses them.

ghostwords · 2017-05-23T20:24:04Z

src/tests/tests/storage.js

  QUnit.test("blocking still cascades after domain declares DNT", (assert) => {
+    let settings_map = storage.getBadgerStorageObject('settings_map');
+    settings_map.setItem("checkForDNTPolicy", true);


Ditto: unnecessary, remove?

ghostwords · 2017-05-23T20:24:08Z

src/tests/tests/storage.js

@@ -312,6 +333,8 @@
  });

  QUnit.test("user actions overrule everything else", (assert) => {
+    let settings_map = storage.getBadgerStorageObject('settings_map');
+    settings_map.setItem("checkForDNTPolicy", true);


Ditto: unnecessary, remove?

ghostwords · 2017-05-23T20:26:45Z

src/tests/tests/storage.js

@@ -191,7 +193,26 @@
    );
  });

+  QUnit.test("DNT does not return as an action if user has chosen not to", (assert) => {


This test doesn't seem to test anything: try getting it to fail by removing the "is DNT checking enabled" checks in the code. The test will still pass because DOMAIN is always seen as NO_TRACKING, not DNT.

hmm. this test works in the suite, but does not work on its own because I mistakenly depended on the leftover state of a previous test.

QUnit.test("DNT does not cascade", (assert) => { runs storage.setupDNT(DOMAIN);

because that marks down DOMAIN as having a DNT policy, the next test disabling it does test it. I'll add that setupDNT to the disable test explicitly.

edit: wait... no... I already added that. now I'm confused. I'm running storage.setupDNT(DOMAIN);??

Yep, seems fine now, I don't know what I was seeing earlier.

ghostwords · 2017-05-23T20:27:50Z

src/tests/tests/storage.js

+    assert.equal(
+      storage.getAction(DOMAIN),
+      constants.NO_TRACKING,
+      "domain is marked as DNT directly"


Should update the message describing the assertion.

ghostwords · 2017-05-23T20:27:55Z

src/tests/tests/storage.js

+    assert.equal(
+      storage.getBestAction(DOMAIN),
+      constants.NO_TRACKING,
+      "domain is marked as DNT"


Should update the message describing the assertion.

ghostwords · 2017-05-23T20:32:04Z

src/js/storage.js

@@ -99,11 +99,13 @@ BadgerPen.prototype = {
   * @returns {String} the presumed action for this FQDN
   **/
  getAction: function (domain, ignoreDNT) {
+    let checking_dnt = badger.isCheckingDNTPolicyEnabled();


Could avoid making another variable by setting ignoreDNT to true if badger.isCheckingDNTPolicyEnabled() is false.

ghostwords · 2017-05-23T20:53:26Z

src/_locales/en_US/messages.json

@@ -89,6 +89,12 @@
    "options_title": {
        "message": "Privacy Badger Options"
    },
+    "options_dnt_policy_setting": {
+        "message": "Check if sites have a DNT policy and unblock them"


How about "Check if sites comply with EFF's Do Not Track policy"?

sure.

BTW. maybe we should change that "learn more" link to this: https://www.eff.org/pages/understanding-effs-do-not-track-policy-universal-opt-out-tracking ?

I dunno, https://www.eff.org/dnt-policy links there already. We might add some more explanatory text to this checkbox in the future too, but no need to wait on that I think.

this should be fixed in a different way.

ghostwords · 2017-05-23T23:02:56Z

Let me know if this is ready for me to look over again.

Ghoughpteighbteau · 2017-05-23T23:17:01Z

ah, yes it is. Could you double check your findings on that unit test? I was able to get it to fail by removing the check in getAction.

Feedback addressed.

ghostwords · 2017-05-25T00:09:07Z

Looks good, thank you! I'll open a pull request with some follow up tweaks.

ghostwords · 2017-05-25T20:39:31Z

@Ghoughpteighbteau Take a look at my follow-up in #1400 if you like.

added option to disable DNT policy checking

dde4be7

Alex Bruce added 2 commits May 22, 2017 18:15

fixed whitespace issue

a303ea2

DNT checking should be *true* by default. oops. added a test for that.

ac7b87e

ghostwords previously requested changes May 23, 2017

View reviewed changes

Alex Bruce added 6 commits May 23, 2017 14:29

corrected copy paste errors in storage unit tests

f0a0f66

Updated wording for check dnt option

2137b18

fixed a syntax error in the anchor tag.

6d2257d

just update ignoreDNT instead of making a new variable

b76a326

moved the set/unset to the function that was using it.

a873df6

removed line that ensures DNT checking happens in test

bf7f9a8

this should be fixed in a different way.

ghostwords self-requested a review May 24, 2017 22:48

ghostwords approved these changes May 25, 2017

View reviewed changes

ghostwords merged commit f6a67f6 into EFForg:master May 25, 2017

ghostwords mentioned this pull request May 25, 2017

Prevent unit tests from modifying local Badger settings #1400

Merged

ghostwords mentioned this pull request May 31, 2018

Add option to disable sending the Do Not Track (DNT) signal #2038

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added option to disable DNT policy checking #1391

added option to disable DNT policy checking #1391

Ghoughpteighbteau commented May 22, 2017 •

edited by ghostwords

Loading

ghostwords commented May 23, 2017

ghostwords commented May 23, 2017 •

edited

Loading

Ghoughpteighbteau commented May 23, 2017 •

edited

Loading

ghostwords May 23, 2017

ghostwords May 23, 2017

Ghoughpteighbteau May 23, 2017

ghostwords May 23, 2017

Ghoughpteighbteau May 23, 2017

ghostwords May 24, 2017

ghostwords May 24, 2017

ghostwords May 23, 2017

Ghoughpteighbteau May 23, 2017 •

edited

Loading

ghostwords May 23, 2017

ghostwords May 23, 2017

ghostwords May 23, 2017

ghostwords May 23, 2017

Ghoughpteighbteau May 23, 2017 •

edited

Loading

ghostwords May 24, 2017

ghostwords May 23, 2017

ghostwords May 23, 2017

ghostwords May 23, 2017

ghostwords May 23, 2017

Ghoughpteighbteau May 23, 2017

ghostwords May 23, 2017

ghostwords commented May 23, 2017

Ghoughpteighbteau commented May 23, 2017

ghostwords commented May 25, 2017 •

edited

Loading

ghostwords commented May 25, 2017 •

edited

Loading


		assert.expect(NUM_TESTS);

		badger.isCheckingDNTPolicyEnabled = () => false;

added option to disable DNT policy checking #1391

added option to disable DNT policy checking #1391

Conversation

Ghoughpteighbteau commented May 22, 2017 • edited by ghostwords Loading

ghostwords commented May 23, 2017

ghostwords commented May 23, 2017 • edited Loading

Ghoughpteighbteau commented May 23, 2017 • 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

Choose a reason for hiding this comment

Ghoughpteighbteau May 23, 2017 • 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

Ghoughpteighbteau May 23, 2017 • 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

Choose a reason for hiding this comment

ghostwords commented May 23, 2017

Ghoughpteighbteau commented May 23, 2017

ghostwords commented May 25, 2017 • edited Loading

ghostwords commented May 25, 2017 • edited Loading

Ghoughpteighbteau commented May 22, 2017 •

edited by ghostwords

Loading

ghostwords commented May 23, 2017 •

edited

Loading

Ghoughpteighbteau commented May 23, 2017 •

edited

Loading

Ghoughpteighbteau May 23, 2017 •

edited

Loading

Ghoughpteighbteau May 23, 2017 •

edited

Loading

ghostwords commented May 25, 2017 •

edited

Loading

ghostwords commented May 25, 2017 •

edited

Loading