Closed Bug 1257556 Opened 8 years ago Closed 8 years ago

Generalize Kinto blocklist Certificate client to addons/plugins/gfx

Categories

(Cloud Services :: Firefox: Common, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

References

Details

Attachments

(2 files, 7 obsolete files)

58 bytes, text/x-review-board-request
MattN
: review+
Details
58 bytes, text/x-review-board-request
MattN
: review+
Details
      No description provided.
Attachment #8731726 - Flags: review?(mgoodwin)
See Also: → 1257533
Blocks: 1257565
Assignee: nobody → mathieu
See Also: → 1224528
Comment on attachment 8731726 [details] [diff] [review]
Generalize Kinto blocklist client to addons/plugins/gfx

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

Looks good to me. You'll also need to get a services peer to review (rnewman, perhaps?).

::: browser/app/profile/firefox.js
@@ +67,5 @@
>  pref("services.kinto.changes.path", "/buckets/monitor/collections/changes/records");
>  pref("services.kinto.bucket", "blocklists");
>  pref("services.kinto.onecrl.collection", "certificates");
>  pref("services.kinto.onecrl.checked", 0);
> +pref("services.kinto.addons.collection", "addons");

Maybe now is not the time to address this but, perhaps, once we're happy this works for Firefox and mobile, we could move these to all.js
Attachment #8731726 - Flags: review?(mgoodwin) → review+
Attachment #8731726 - Flags: review?(rnewman)
Comment on attachment 8731726 [details] [diff] [review]
Generalize Kinto blocklist client to addons/plugins/gfx

Matt, dolske suggested that you'd be a good person to get acquainted with this stuff.
Attachment #8731726 - Flags: review?(rnewman) → review?(MattN+bmo)
Status: NEW → ASSIGNED
Comment on attachment 8731726 [details] [diff] [review]
Generalize Kinto blocklist client to addons/plugins/gfx

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

Pleas use a Git/HG move from services/common/KintoCertificateBlocklist.js to services/common/KintoBlocklist.js so it's easier to review what actually changed.
Review commit: https://reviewboard.mozilla.org/r/44297/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44297/
Attachment #8738085 - Attachment description: MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx r?MattN → MozReview Request: Bug 1257556 - Rename KintoBlocklist client
Attachment #8738091 - Flags: review?(MattN+bmo)
Comment on attachment 8738085 [details]
MozReview Request: Bug 1257556 - Rename KintoBlocklist client,r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44295/diff/1-2/
Attachment #8731726 - Attachment is obsolete: true
Attachment #8731726 - Flags: review?(MattN+bmo)
Blocks: 1263602
Attachment #8738085 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8738085 [details]
MozReview Request: Bug 1257556 - Rename KintoBlocklist client,r=MattN

https://reviewboard.mozilla.org/r/44295/#review42849
Comment on attachment 8738091 [details]
MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx,r=MattN

https://reviewboard.mozilla.org/r/44297/#review42853

Apologies again for the delay. r=me with some changes and some discussion points below.

::: services/common/KintoBlocklist.js:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -this.EXPORTED_SYMBOLS = ["OneCRLClient"];
> +this.EXPORTED_SYMBOLS = ["AddonClient", "GfxClient", "OneCRLClient", "PluginClient"];

Some of these symbols aren't descriptive enough. I think they should have blocklist or somethng like that in the name. If I saw code using `AddonClient` I wouldn't suspect it has anything to do with the blocklist.

::: services/common/KintoBlocklist.js:63
(Diff revision 1)
> -    let updateLastCheck = function() {
> -      let checkedServerTimeInSeconds = Math.round(serverTime / 1000);
> -      Services.prefs.setIntPref(PREF_KINTO_ONECRL_CHECKED_SECONDS,
> -                                checkedServerTimeInSeconds);
> +
> +class BlocklistClient {
> +
> +  constructor(collectionName, checkPrefName, processCallback) {
> +    this.collectionName = collectionName;
> +    this.checkPrefName = checkPrefName;

I find checkPrefName to be a bit vague for the property name.

(I actually don't like the "checked" naming anywhere as it's also vague and I believe "updated" would be more descriptive and accurate IIUC. I'm not sure if it's too late to fix that now?)

Since there is no such thing as a reference to a preference I think the "Name" suffix can be dropped so it's not too long. How about "updateTimePref" or "lastUpdatePref"? Or if you're using "check" instead of "update" since there may be a check with no actual change then perhaps "lastCheckTimePref"?

::: services/common/KintoBlocklist.js:71
(Diff revision 1)
> +                                the remote collection.
> +   * @param {Date} serverTime   the current date return by server.

Nit: "returned by the server"

::: services/common/KintoBlocklist.js:78
(Diff revision 1)
> +
> +    return Task.spawn((function* () {
>        try {

Nit: Adding a function name here can be useful for stack traces

::: services/common/KintoBlocklist.js:94
(Diff revision 1)
> +
> +        yield this.processCallback(list.data);
> +
> +        // Track last update.
> +        this.updateLastCheck(serverTime);

Is there ever a case where we would want an exception in the callback to prevent the updateLastCheck call? If not, I suggest that the try…catch move from updateJSONBlocklist to here.

::: services/common/KintoBlocklist.js:148
(Diff revision 1)
> +  try {
> +    yield OS.File.writeAtomic(path, serialized, {tmpPath: path + ".tmp"});
>    }
> +  catch(e) {
> +    Cu.reportError(e);
> +  }
> +
> +  // Notify change to `nsBlocklistService`
> +  const eventData = {filename: filename};
> +  Services.cpmm.sendAsyncMessage("Blocklist:reload-from-disk", eventData);

If we failed writing the new file, I don't think we would want to notify about a change (which didn't happen).

::: services/common/KintoBlocklist.js:150
(Diff revision 1)
>    }
> +  catch(e) {

Nit: cuddle the catch

::: services/common/kinto-updater.js:83
(Diff revision 1)
> -kintoClients.certificates =
> -  Cu.import("resource://services-common/KintoCertificateBlocklist.js", {})
> -  .OneCRLClient;
> +const KintoBlocklist = Cu.import("resource://services-common/KintoBlocklist.js", {});
> +gKintoClients[Services.prefs.getCharPref(PREF_KINTO_ONECRL_COLLECTION)]  = KintoBlocklist.OneCRLClient;
> +gKintoClients[Services.prefs.getCharPref(PREF_KINTO_ADDONS_COLLECTION)]  = KintoBlocklist.AddonClient;
> +gKintoClients[Services.prefs.getCharPref(PREF_KINTO_GFX_COLLECTION)]     = KintoBlocklist.GfxClient;
> +gKintoClients[Services.prefs.getCharPref(PREF_KINTO_PLUGINS_COLLECTION)] = KintoBlocklist.PluginClient;

Nit: You could use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#Computed_property_names for the gKintoClients properties. That would at least keep the const defintion closer to the properties and make these lines less verbose.

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:1
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Nit: Tests are public domain by default nowadays so this isn't necessary.

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:7
(Diff revision 1)
> +const FILE_BLOCKLIST_ADDONS  = "blocklist-addons.json";
> +const FILE_BLOCKLIST_GFX     = "blocklist-gfx.json";
> +const FILE_BLOCKLIST_PLUGINS = "blocklist-plugins.json";

```const {FILENAME_ADDONS_JSON, FILENAME_GFX_JSON, …} = Cu.import("resource://services-common/KintoBlocklist.js", {});```
to avoid duplication.

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:10
(Diff revision 1)
> +const PREF_KINTO_ADDONS_CHECKED_SECONDS  = "services.kinto.addons.checked";
> +const PREF_KINTO_GFX_CHECKED_SECONDS     = "services.kinto.gfx.checked";
> +const PREF_KINTO_PLUGINS_CHECKED_SECONDS = "services.kinto.plugins.checked";

Nit: You could avoid duplicating this by getting it from the exported clients' `checkPrefName` property.

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:25
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "FileUtils",
> +                                  "resource://gre/modules/FileUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "OS",
> +                                  "resource://gre/modules/osfile.jsm");

Nit: No need to lazy-load stuff in tests or in-general when you know it's going to get loaded right away anyways.

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:30
(Diff revision 1)
> +var server;
> +var kintoClient;

Don't use `var` in new code (regardless of scope), only `const` or `let`.

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:47
(Diff revision 1)
> +}
> +
> +
> +function* readJSON(filepath) {

Nit: extra new line

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:54
(Diff revision 1)
> +}
> +
> +
> +function* clear_state() {

Nit: extra newline

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:65
(Diff revision 1)
> +  if (blocklist.exists())
> +    blocklist.remove(true);

It's prefered to always brace if/else in new code.

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:75
(Diff revision 1)
> +  blocklist = FileUtils.getFile(KEY_PROFILEDIR, [FILE_BLOCKLIST_GFX]);
> +  if (blocklist.exists())
> +    blocklist.remove(true);
> +
> +  // Clear local DBs.
> +  for(let name of ["addons", "gfx", "plugins"]) {

Nit: missing space after `for`

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:92
(Diff revision 1)
> +function run_test() {
> +  // Set up an HTTP Server
> +  server = new HttpServer();
> +  server.start(-1);
> +
> +  // Point the KintoAddonPlugin client to use this local HTTP server.

I think `KintoAddonPlugin` is a leftover name? I guess it's also used by this test file to describe what it's testing as I guess it doesn't test oneCRL stuff?

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:97
(Diff revision 1)
> +  // Point the KintoAddonPlugin client to use this local HTTP server.
> +  Services.prefs.setCharPref("services.kinto.base",
> +                             `http://localhost:${server.identity.primaryPort}/v1`);
> +
> +  // Setup server fake responses.
> +  function handleResponse (request, response) {

Nit: remove the space before `(`

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:99
(Diff revision 1)
> +      const sampled = getSampleResponse(request, server.identity.primaryPort);
> +      if (!sampled) {

Nit: `sample`?

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:113
(Diff revision 1)
> +      response.write(sampled.responseBody);
> +    } catch (e) {

I think you're missing response.finish()

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:115
(Diff revision 1)
> +      }
> +      response.setHeader("Date", (new Date()).toUTCString());
> +
> +      response.write(sampled.responseBody);
> +    } catch (e) {
> +      dump(`${e}\n`);

I think do_print is preferred and it includes the \n for you

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:130
(Diff revision 1)
> +  do_register_cleanup(function() {
> +    server.stop(function() { });

Nit: if you're creating an anonymous function you can use fat arrow functions: =>

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:174
(Diff revision 1)
> +});
> +
> +add_task(clear_state);
> +
> +
> +add_task(function* test_plugins_list_is_written_to_file_in_profile(){

Nit: I wouldn't put a newline before the clear_state lines and only have one after

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:197
(Diff revision 1)
> +
> +add_task(function* test_current_server_time_is_saved_in_addons_pref(){
> +  const before = Services.prefs.getIntPref(PREF_KINTO_ADDONS_CHECKED_SECONDS);
> +  yield AddonClient.maybeSync(2000, Date.now());
> +  const after = Services.prefs.getIntPref(PREF_KINTO_ADDONS_CHECKED_SECONDS);
> +  notEqual(before, after);

I think you should do a sanity check on the value e.g. check that it's a date from the last 5 minutes. Same as the others below

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:288
(Diff revision 1)
> +        received = aMsg;
> +        resolve();

Why not just resolve(aMsg) to avoid a variable and some lines:

let received = yield new Promise((resolve, reject) => {

::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:303
(Diff revision 1)
> +});
> +
> +add_task(clear_state);
> +
> +
> +add_task(function* test_sends_reload_message_when_plugins_has_changes(){

I'm not sure it's necessary to separately test each client when they share the same code.
Attachment #8738091 - Flags: review?(MattN+bmo) → review+
https://reviewboard.mozilla.org/r/44297/#review42853

> I find checkPrefName to be a bit vague for the property name.
> 
> (I actually don't like the "checked" naming anywhere as it's also vague and I believe "updated" would be more descriptive and accurate IIUC. I'm not sure if it's too late to fix that now?)
> 
> Since there is no such thing as a reference to a preference I think the "Name" suffix can be dropped so it's not too long. How about "updateTimePref" or "lastUpdatePref"? Or if you're using "check" instead of "update" since there may be a check with no actual change then perhaps "lastCheckTimePref"?

For now, I renamed to lastCheckTimePref.

As for the «check» notion, we can change that in another contribution (since it will conflict with other patches about to land). BTW it is a bit different than «update», it is more like a «ping». We'll think of something to make this crystal clear.

Also, we may want to abstract a bit the notion «kinto» (eg. «storage») and add more namespacing in the pref name (eg. "services.kinto.onecrl.checked" → "services.storage.blocklist.onecrl.checked")

> Is there ever a case where we would want an exception in the callback to prevent the updateLastCheck call? If not, I suggest that the try…catch move from updateJSONBlocklist to here.

Yes, if an exception occurs then we don't want to update the last check.

> I'm not sure it's necessary to separately test each client when they share the same code.

I refactored the tests to avoid duplication!
I'd prefer to keep some tests that assert the parameters they were instantiated with.
Attachment #8741703 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8741703 [details]
MozReview Request: Bug 1257556 - Address some of MattN comments,r=MattN

https://reviewboard.mozilla.org/r/46695/#review43435

::: services/common/KintoBlocklist.js:11
(Diff revision 1)
> +                         "FILENAME_ADDONS_JSON",
> +                         "FILENAME_GFX_JSON",
> +                         "FILENAME_PLUGINS_JSON"];

Note that I wasn't suggesting that you export these, I was suggesting that the test reach into the BackstagePass and use them without exporting. I think it's fine to do that from a test for the specific module.

::: services/common/kinto-updater.js:32
(Diff revision 1)
> +// Add a blocklist client for testing purposes. Do not use for any other purpose
> +this.addTestKintoClient = (name, client) => { gBlocklistClients[name] = client; }
> +
>  // This is called by the ping mechanism.
>  // returns a promise that rejects if something goes wrong
> -this.checkVersions = function() {
> +this.checkVersions = function checkVersions() {

In this case the JS engine can already figure out the name from the LHS so you don't need the name with the function.
Attachment #8741716 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8741716 [details]
MozReview Request: Bug 1257556 - Rename checkPrefName to lastCheckTimePref,r=MattN

https://reviewboard.mozilla.org/r/46705/#review43437
Attachment #8741726 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8741726 [details]
MozReview Request: Bug 1257556 - Rename blocklist clients instances,r=MattN

https://reviewboard.mozilla.org/r/46707/#review43439
Comment on attachment 8741798 [details]
MozReview Request: Bug 1257556 - Add proper tests for time saved in preferences,r=MattN

https://reviewboard.mozilla.org/r/46761/#review43441
Attachment #8741798 - Flags: review?(MattN+bmo) → review+
Attachment #8741799 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8741799 [details]
MozReview Request: Bug 1257556 - Refactor tests to avoid duplication between clients,r=MattN

https://reviewboard.mozilla.org/r/46763/#review43443

Note that you don't need to flag me for review again if I give a "Ship it with issues". You simply fix the issues then push back to review with r=… and then land/checkin-needed.
https://reviewboard.mozilla.org/r/46695/#review43435

> Note that I wasn't suggesting that you export these, I was suggesting that the test reach into the BackstagePass and use them without exporting. I think it's fine to do that from a test for the specific module.

I think it would still be useful to have those constants available in order to use them in `nsBlocklistService.js` when loading them during startup.
Comment on attachment 8738091 [details]
MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx,r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44297/diff/1-2/
Attachment #8738091 - Attachment description: MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx r?MattN → MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx r=MattN
Attachment #8741703 - Attachment is obsolete: true
Attachment #8741716 - Attachment is obsolete: true
Attachment #8741726 - Attachment is obsolete: true
Attachment #8741798 - Attachment is obsolete: true
Attachment #8741799 - Attachment is obsolete: true
Attachment #8742285 - Attachment is obsolete: true
Attachment #8742285 - Flags: review?(MattN+bmo)
Comment on attachment 8738091 [details]
MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx,r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44297/diff/2-3/
Hi Mathieu,

I was going to land this but there are conflicts with some that I already landed. I started to resolve the conflicts myself but I'm afraid I'll make a mistake. Can you please rebase this on a more recent revision then re-push that back here so it's ready for review (e.g. with r=MattN in the commit messages). Then you can add the `checkin-needed` bug keyword.
Flags: needinfo?(mathieu)
Blocks: 1266235
Comment on attachment 8738085 [details]
MozReview Request: Bug 1257556 - Rename KintoBlocklist client,r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44295/diff/2-3/
Attachment #8738085 - Attachment description: MozReview Request: Bug 1257556 - Rename KintoBlocklist client → MozReview Request: Bug 1257556 - Rename KintoBlocklist client,r=MattN
Attachment #8738091 - Attachment description: MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx r=MattN → MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx,r=MattN
Comment on attachment 8738091 [details]
MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx,r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44297/diff/3-4/
Flags: needinfo?(mathieu)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/102f375bffd1
https://hg.mozilla.org/mozilla-central/rev/253a857a1dcc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1266794
You need to log in before you can comment on or make changes to this bug.