Use extension for data file updates #76

emerick · 2018-03-29T15:11:38Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions

bbondy · 2018-03-29T15:12:10Z

browser/extensions/brave_component_installer.cc

@@ -0,0 +1,150 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.


MPL-2.0 headers for new files

bbondy · 2018-03-29T15:12:27Z

browser/extensions/brave_component_installer.cc

+using component_updater::ComponentUpdateService;
+
+namespace {
+


nit: remove extra newline

bbondy · 2018-03-29T15:14:51Z

components/brave_shields/browser/ad_block_service.h

 #include "brave/components/brave_shields/browser/base_brave_shields_service.h"
 #include "content/public/common/resource_type.h"

 class AdBlockClient;
 class AdBlockServiceTest;

+const uint8_t kAdBlockPlusUpdaterPublicKey[294] = {


Pls move this directly to an encoded string.

simonhong · 2018-03-29T21:14:26Z

IIUC, src/brave/components and src/components are equivalent layer.
If so, accessing directly to chrome layer from components layer is layer violation.

emerick · 2018-03-30T13:40:35Z

@simonhong Thanks for pointing that out, I didn't realize that. I assume you're referring to my use of g_browser_process? I take it the appropriate way to do this would be to pass the browser process in when constructing the ad block service? It seems that several files in that directory are breaking this rule, unfortunately.

simonhong · 2018-03-30T19:38:57Z

Referring src/brave/browser is same.
I think src/brave/browser and src/chrome/browser are same layer, too.

brave/components/brave_shields/browser has dependency to chrome/common.
I think this dependency should be removed, too if we want to use src/brave/components as src/components.

We should add dependency check rules to modules that we created.

bbondy · 2018-03-31T04:41:28Z

I always viewed src/brave/* as equivalent to something that would be in src/chrome/* and then browser files can use browser and common chrome files. Renderer can use renderer and common files.

bbondy · 2018-03-31T04:47:43Z

components/brave_shields/browser/ad_block_service.h

+  void OnComponentReady(
+    const std::string& extension_id,
+    const base::FilePath& install_dir);
+
  friend class ::AdBlockServiceTest;
  static GURL g_ad_block_url;


I think you can just get rid of this g_ad_block_url now

bbondy · 2018-03-31T04:54:46Z

components/brave_shields/browser/base_brave_shields_service.h

-  }
+//  scoped_refptr<base::SequencedTaskRunner> GetTaskRunner() {
+//    return task_runner_;
+//  }


This is used by the browser tests but those will need to be updated anyway. You can just remove the code for it.

bbondy · 2018-03-31T04:55:41Z

components/brave_shields/browser/base_brave_shields_service.h

  bool initialized_;
-  std::unique_ptr<DATFileWebRequest> web_request_;
  std::mutex init_mutex_;


I don't think you'll need the init_mutex

simonhong · 2018-04-01T21:09:05Z

@bbondy If we think src/brave and src/chrome as a equivalent layer, where is the proper place if we want to add new modules to content layer or components layer?

bbondy · 2018-04-01T22:22:19Z

I think we should use src/chromium_src for that. It is special and has include path equivalent to src. It also supports overwriting .cc files if they happen to match the equivalent files in src, which should very infrequently be used, but can be useful for cases like a config only file.

bbondy · 2018-04-02T03:30:06Z

For anything outside of this, just post a new issue and we can start to move thing that apply to chromium_src if others agree.

bbondy · 2018-04-16T15:12:59Z

components/brave_shields/browser/ad_block_service.h

+    "VwIDAQAB";
+
+const std::string kAdBlockPlusUpdaterId("cffkpbalmllkdoenhmdmpbkajipdjfam");
+const std::string kAdBlockPlusUpdaterName("AdBlock Plus Updater");


The filter syntax that this library uses is AdBlockPlus Filter syntax, but the actual library it uses has nothing to do with AdBlockPlus so please call this:
Brave Ad Block Updater

bbondy · 2018-04-16T15:14:16Z

components/brave_shields/browser/ad_block_service.h

-  static void SetAdBlockURLForTest(const GURL& url);
+  void OnComponentRegistered(const std::string& extension_id);
+  void OnComponentReady(const std::string& extension_id,
+                        const base::FilePath& install_dir);


Let's make these overrides here, and in the base virtual.

bbondy · 2018-04-16T15:15:00Z

components/brave_shields/browser/ad_block_service.cc

+               base::Unretained(this), kAdBlockPlusUpdaterId);
+  brave::RegisterComponent(g_browser_process->component_updater(),
+      kAdBlockPlusUpdaterName, kAdBlockPlusUpdaterBase64PublicKey,
+      registered_callback, ready_callback);


Pls handle all of this in the base, and then pass kAdBlockPlusUpdaterName, kAdBlockPlusUpdaterBase64PublicKey, and kAdBlockPlusUpdaterId to the base class.

bbondy · 2018-04-16T15:31:42Z

components/brave_shields/browser/https_everywhere_service.cc

+  brave::RegisterComponent(g_browser_process->component_updater(),
+      kHTTPSEverywhereUpdaterName, kHTTPSEverywhereUpdaterBase64PublicKey,
+      registered_callback, ready_callback);
+  return true;


Per above, this will be removed after that change.

bbondy · 2018-04-16T15:31:56Z

components/brave_shields/browser/https_everywhere_service.cc

 }

 bool HTTPSEverywhereService::GetHTTPSURL(
    const GURL* url, const uint64_t& request_identifier,
    std::string& new_url) {
  base::AssertBlockingAllowed();
-  if (!IsInitialized() || url->scheme() == "https") {
+  if (!IsInitialized() || url->scheme() == url::kHttpsScheme) {


bbondy · 2018-04-16T15:34:36Z

components/brave_shields/browser/tracking_protection_service.cc

+  brave::RegisterComponent(g_browser_process->component_updater(),
+      kTrackingProtectionUpdaterName, kTrackingProtectionUpdaterBase64PublicKey,
+      registered_callback, ready_callback);
+  return true;


Ditto above, this will be common code.

bbondy · 2018-04-16T15:40:16Z

test/data/adblock-data/manifest.json

@@ -0,0 +1,7 @@
+{


Pls ensure that the keys here are for a different public key than we use in prod. You might need a hack similar to SetAdBlockURLForTest for the tests.

bbondy · 2018-04-16T15:41:26Z

test/data/https-everywhere-data/manifest.json

+  "manifest_version": 2,
+  "name": "HTTPS Everywhere updater",
+  "version": "1.0.0"
+}


Would you mind also doing a test for tracking-protection at this point? Ask Serg if he can give you a test case example that will be blocked by tracking protection library but not the ad-block library.

bbondy · 2018-04-16T15:44:45Z

Great work so far, looks very close.

bbondy · 2018-04-27T17:13:27Z

common/network_constants.cc

@@ -12,6 +14,6 @@ const char kTwitterReferrer[] = "https://twitter.com/*";
 const char kTwitterRedirectURL[] = "https://mobile.twitter.com/i/nojs_router*";

 const char kCookieHeader[] = "Cookie";
-// Intentional mispelling on referrer to match HTTP spec
+// Intentional misspelling on referrer to match HTTP spec


😆 LOL! This is too good.

bbondy · 2018-04-27T17:35:45Z

components/brave_shields/browser/tracking_protection_service_browsertest.cc

+    "eclbkhjphkhalklhipiicaldjbnhdfkc");
+
+const std::string kTrackingProtectionComponentTestBase64PublicKey =
+    "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAsleoSxQ3DN+6xym2P1uX"


Looks like the test manifest is still referencing the non-test public key.

…stered

bbondy · 2018-04-28T00:24:24Z

browser/BUILD.gn

    "importer/brave_external_process_importer_client.cc",
    "importer/brave_external_process_importer_client.h",
    "importer/brave_external_process_importer_host.cc",
    "importer/brave_external_process_importer_host.h",
    "importer/brave_in_process_importer_bridge.cc",
    "importer/brave_in_process_importer_bridge.h",
    "importer/brave_profile_writer.cc",
-    "importer/brave_profile_writer.h",
+    "importer/brave_profile_writer.h"


trailing commas are OK for future ref in .gn files.

not just ok, trailing commas are preferred

bbondy · 2018-04-28T00:24:42Z

🎉

Switch to production

Package updates to fix security issues

symbol_level is set to 1 for x86 Linux

bbondy reviewed Mar 29, 2018

View reviewed changes

bbondy reviewed Mar 31, 2018

View reviewed changes

bbondy assigned emerick Apr 4, 2018

emerick force-pushed the dat-file-extension branch from 882afd0 to 08c2492 Compare April 16, 2018 14:24

bbondy requested changes Apr 16, 2018

View reviewed changes

emerick changed the title ~~WIP: Use extension for data file updates~~ Use extension for data file updates Apr 17, 2018

emerick force-pushed the dat-file-extension branch 2 times, most recently from 238b201 to 20df890 Compare April 26, 2018 16:12

bbondy reviewed Apr 27, 2018

View reviewed changes

emerick added 8 commits April 27, 2018 14:38

Use extension for data file updates

581b1b4

Incorporate code review feedback and get browser tests working again

5238405

Reinitialize clients when consuming a new dat file

f9de927

Incorporate code review feedback

182b52f

Add browser test for tracking protection service

98e22c9

Redirect extension update requests to Brave's extension server

be7c777

Specify correct scheme for fallback component updater URLs

17b725c

Override update_client::Configurator so we can disable CUP signing

da54481

emerick added 2 commits April 27, 2018 14:39

Call OnDemandUpdate to force component install once component is regi…

9ae1117

…stered

Specify test public keys in test manifests

52c516f

emerick force-pushed the dat-file-extension branch from 20df890 to 52c516f Compare April 27, 2018 18:39

Update tracking_protection_service.cc

a4c1718

bbondy reviewed Apr 28, 2018

View reviewed changes

bbondy approved these changes Apr 28, 2018

View reviewed changes

bbondy merged commit 2eac229 into master Apr 28, 2018

NejcZdovc added a commit that referenced this pull request Dec 10, 2018

Merge pull request #76 from brave-intl/production

65b9ec9

Switch to production

bbondy pushed a commit that referenced this pull request Feb 18, 2019

Merge pull request #76 from diracdeltas/fix/npm-audit

3f0561f

Package updates to fix security issues

fmarier pushed a commit that referenced this pull request Oct 29, 2019

Merge pull request #76 from brave/x86_linux_package_sym_level_1

34db500

symbol_level is set to 1 for x86 Linux

petemill pushed a commit that referenced this pull request Jul 27, 2020

Merge pull request #76 from brave/x86_linux_package_sym_level_1

3f0d079

symbol_level is set to 1 for x86 Linux

petemill pushed a commit that referenced this pull request Jul 28, 2020

Merge pull request #76 from brave/x86_linux_package_sym_level_1

17ea455

symbol_level is set to 1 for x86 Linux

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use extension for data file updates #76

Use extension for data file updates #76

emerick commented Mar 29, 2018 •

edited

Loading

bbondy Mar 29, 2018

bbondy Mar 29, 2018

bbondy Mar 29, 2018

simonhong commented Mar 29, 2018

emerick commented Mar 30, 2018

simonhong commented Mar 30, 2018 •

edited

Loading

bbondy commented Mar 31, 2018

bbondy Mar 31, 2018

bbondy Mar 31, 2018

bbondy Mar 31, 2018

simonhong commented Apr 1, 2018

bbondy commented Apr 1, 2018

bbondy commented Apr 2, 2018 •

edited

Loading

bbondy Apr 16, 2018

bbondy Apr 16, 2018

bbondy Apr 16, 2018

bbondy Apr 16, 2018

bbondy Apr 16, 2018

bbondy Apr 16, 2018

bbondy Apr 16, 2018

bbondy Apr 16, 2018

bbondy commented Apr 16, 2018

bbondy Apr 27, 2018

bbondy Apr 27, 2018

bbondy Apr 28, 2018

bridiver Feb 18, 2020

bbondy commented Apr 28, 2018

		using component_updater::ComponentUpdateService;

		namespace {

Use extension for data file updates #76

Use extension for data file updates #76

Conversation

emerick commented Mar 29, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Mar 29, 2018

emerick commented Mar 30, 2018

simonhong commented Mar 30, 2018 • edited Loading

bbondy commented Mar 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Apr 1, 2018

bbondy commented Apr 1, 2018

bbondy commented Apr 2, 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

Choose a reason for hiding this comment

bbondy commented Apr 16, 2018

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

bbondy commented Apr 28, 2018

emerick commented Mar 29, 2018 •

edited

Loading

simonhong commented Mar 30, 2018 •

edited

Loading

bbondy commented Apr 2, 2018 •

edited

Loading