Bug 1548631 - Add capability checks before evaluating Normandy recipes r=Gijs,glasserc

Differential Revision: https://phabricator.services.mozilla.com/D36850

--HG--
extra : moz-landing-system : lando

toolkit/components/normandy/docs/execution-model.rst

@@ -0,0 +1,85 @@
+Execution Model
+===============
+This document describes the execution model of the Normandy Client.
+
+The basic unit of instruction from the server is a *recipe*, which contains
+instructions for filtering and arguments for a given action. See below for
+details.
+
+One iteration through all of these steps is called a *Normandy session*. This
+happens at least once every 6 hours, and possibly more often if Remote
+Settings syncs new changes.
+
+1. Fetching
+-----------
+A list of all active recipes is retrieved from the Normandy service. This
+happens one of two ways, depending on which features have been enabled.
+
+- Recipes are retrieved from Remote Settings, which has likely been syncing
+  them in the background. This is the default behavior.
+- Recipes are fetched directly from the Normandy server. This is legacy
+  behavior.
+
+Once recipes have been retrieved, they are validated using a signature
+generated by Autograph_ that is included with the recipe. This signature
+validates both the contents of the recipe as well as its source.
+
+.. _Autograph: https://github.com/mozilla-services/autograph
+
+2. Filtering
+------------
+Recipes contain information about which clients should execute the recipe.
+All recipes are processed by all clients, and all filtering happens in the
+client. There are two types of filters: capabilities and filter expressions.
+
+Capabilities
+~~~~~~~~~~~~
+First a recipe is checked for compatibility using *capabilities*.
+Capabilities are simple strings, such as ``"action:show-heartbeat"``. A
+recipe contains a list of required capabilities, and the Normandy Client has
+a list of capabilities that it supports. If any of the capabilities required
+by the recipe are not compatible with the client, then the recipe does not
+execute.
+
+Capabilities are used to avoid running recipes on a client that are so
+incompatible as to be harmful. For example, some changes to filter expression
+handling cannot be detected by filter expressions, and so older clients that
+receive filters using these new features would break.
+
+.. note:
+
+    Capabilities were first introduced in Firefox 70. Clients prior to this do
+    not check capabilities, and run all recipes provided. This will require some
+    workaround until we can either segment the recipe population of new and old
+    clients, or until the older clients age out of the system.
+
+Filter Expressions
+~~~~~~~~~~~~~~~~~~
+Second the recipe's filter expression is checked. Filter expressions are
+written in an expression language named JEXL_ that is similar to JavaScript,
+but far simpler. It is intended to be as safe to evaluate as possible.
+
+.. _JEXL: https://github.com/mozilla/mozjexl
+
+Filters are evaluated in a context that contains details about the client
+including browser versions, installed add-ons, and Telemetry data. Filters
+have access to "transforms" which are simple functions that can do things like
+check preference values or parse strings into ``Date`` objects. Filters don't
+have access to change any state in the browser, and are generally
+idempotent. However, filters are *not* considered to be "pure functions",
+because they have access to state that may change, such as time and location.
+
+3. Execution
+------------
+If a recipe's filters all pass, then the recipe is executed. The recipe
+specifies exactly one *action* by name, as well as arguments to pass to that
+action. The arguments are validated against an expected schema.
+
+All action have a pre- and post-step that runs once each Normandy session.
+The pre-step is run before any recipes are executed, and once the post-step
+is executed, no more recipes will be executed on that action in this session.
+
+Each recipe is passed to the action. Individual actions have their own
+semantics about what to do with recipes. Many actions maintain their own life
+cycle of events for new recipes, existing recipes, and recipes that stop
+applying to this client.

toolkit/components/normandy/docs/index.rst

@@ -27,3 +27,4 @@ and then executes them.
    :maxdepth: 1
 
    data-collection
+   execution-model

toolkit/components/normandy/lib/ActionsManager.jsm

@@ -30,6 +30,23 @@ var EXPORTED_SYMBOLS = ["ActionsManager"];
 
 const log = LogManager.getLogger("recipe-runner");
 
+const actionConstructors = {
+  "addon-study": AddonStudyAction,
+  "branched-addon-study": BranchedAddonStudyAction,
+  "console-log": ConsoleLogAction,
+  "multi-preference-experiment": PreferenceExperimentAction,
+  "preference-rollback": PreferenceRollbackAction,
+  "preference-rollout": PreferenceRolloutAction,
+  "show-heartbeat": ShowHeartbeatAction,
+  "single-preference-experiment": SinglePreferenceExperimentAction,
+};
+
+// Legacy names used by the server and older clients for actions.
+const actionAliases = {
+  "opt-out-study": "addon-study",
+  "preference-experiment": "single-preference-experiment",
+};
+
 /**
  * A class to manage the actions that recipes can use in Normandy.
  */
@@ -37,22 +54,27 @@ class ActionsManager {
   constructor() {
     this.finalized = false;
 
-    const addonStudyAction = new AddonStudyAction();
-    const singlePreferenceExperimentAction = new SinglePreferenceExperimentAction();
+    // Build a set of local actions, and aliases to them. The aliased names are
+    // used by the server to keep compatibility with older clients.
+    this.localActions = {};
+    for (const [name, Constructor] of Object.entries(actionConstructors)) {
+      this.localActions[name] = new Constructor();
+    }
+    for (const [alias, target] of Object.entries(actionAliases)) {
+      this.localActions[alias] = this.localActions[target];
+    }
+  }
 
-    this.localActions = {
-      "addon-study": addonStudyAction,
-      "branched-addon-study": new BranchedAddonStudyAction(),
-      "console-log": new ConsoleLogAction(),
-      "opt-out-study": addonStudyAction, // Legacy name used for addon-study on Normandy server
-      "multi-preference-experiment": new PreferenceExperimentAction(),
-      // Historically, this name meant SinglePreferenceExperimentAction.
-      "preference-experiment": singlePreferenceExperimentAction,
-      "preference-rollback": new PreferenceRollbackAction(),
-      "preference-rollout": new PreferenceRolloutAction(),
-      "single-preference-experiment": singlePreferenceExperimentAction,
-      "show-heartbeat": new ShowHeartbeatAction(),
-    };
+  static getCapabilities() {
+    // Prefix each action name with "action." to turn it into a capability name.
+    let capabilities = new Set();
+    for (const actionName of Object.keys(actionConstructors)) {
+      capabilities.add(`action.${actionName}`);
+    }
+    for (const actionAlias of Object.keys(actionAliases)) {
+      capabilities.add(`action.${actionAlias}`);
+    }
+    return capabilities;
   }
 
   async runRecipe(recipe) {

toolkit/components/normandy/lib/RecipeRunner.jsm

@@ -63,7 +63,7 @@ const PREFS_TO_WATCH = [
 XPCOMUtils.defineLazyGetter(this, "gRemoteSettingsClient", () => {
   return RemoteSettings(REMOTE_SETTINGS_COLLECTION, {
     filterFunc: async entry =>
-      (await RecipeRunner.checkFilter(entry.recipe)) ? entry : null,
+      (await RecipeRunner.shouldRunRecipe(entry.recipe)) ? entry : null,
   });
 });
 
@@ -285,9 +285,7 @@ var RecipeRunner = {
     timerManager.registerTimer(TIMER_NAME, () => this.run(), runInterval);
   },
 
-  async run(options = {}) {
-    const { trigger = "timer" } = options;
-
+  async run({ trigger = "timer" } = {}) {
     if (this.running) {
       // Do nothing if already running.
       return;
@@ -324,18 +322,18 @@ var RecipeRunner = {
         return;
       }
 
-      const actions = new ActionsManager();
+      const actionsManager = new ActionsManager();
 
       // Execute recipes, if we have any.
       if (recipesToRun.length === 0) {
         log.debug("No recipes to execute");
       } else {
         for (const recipe of recipesToRun) {
-          await actions.runRecipe(recipe);
+          await actionsManager.runRecipe(recipe);
         }
       }
 
-      await actions.finalize();
+      await actionsManager.finalize();
 
       await Uptake.reportRunner(Uptake.RUNNER_SUCCESS);
       Services.obs.notifyObservers(null, "recipe-runner:end");
@@ -355,9 +353,9 @@ var RecipeRunner = {
    */
   async loadRecipes() {
     // If RemoteSettings is enabled, we read the list of recipes from there.
-    // The JEXL filtering is done via the provided callback (see `gRemoteSettingsClient`).
+    // The recipe filtering is done via the provided callback (see `gRemoteSettingsClient`).
     if (this.loadFromRemoteSettings) {
-      // First, fetch recipes whose JEXL filters match.
+      // First, fetch recipes that should run on this client.
       const entries = await gRemoteSettingsClient.get();
       // Then, verify the signature of each recipe. It will throw if invalid.
       return Promise.all(
@@ -381,10 +379,13 @@ var RecipeRunner = {
       log.error(`Could not fetch recipes from ${apiUrl}: "${e}"`);
       throw e;
     }
-    // Evaluate recipe filters
+
+    // Check if each recipe should be run, according to `shouldRunRecipe`. This
+    // can't be a simple call to `Array.filter` because checking if a recipe
+    // should run is an async operation.
     const recipesToRun = [];
     for (const recipe of recipes) {
-      if (await this.checkFilter(recipe)) {
+      if (await this.shouldRunRecipe(recipe)) {
         recipesToRun.push(recipe);
       }
     }
@@ -405,13 +406,72 @@ var RecipeRunner = {
   },
 
   /**
-   * Evaluate a recipe's filter expression against the environment.
+   * Return the set of capabilities this runner has.
+   *
+   * This is used to pre-filter recipes that aren't compatible with this client.
+   *
+   * @returns {Set<String>} The capabilities supported by this client.
+   */
+  getCapabilities() {
+    let capabilities = new Set([
+      "capabilities-v1", // The initial version of the capabilities system.
+    ]);
+
+    // Get capabilities from ActionsManager.
+    for (const actionCapability of ActionsManager.getCapabilities()) {
+      capabilities.add(actionCapability);
+    }
+
+    // Add a capability for each transform available to JEXL.
+    for (const transform of FilterExpressions.getAvailableTransforms()) {
+      capabilities.add(`jexl.transform.${transform}`);
+    }
+
+    return capabilities;
+  },
+
+  /**
+   * Decide if a recipe should be run.
+   *
+   * This checks two things in order: capabilities, and filter expression.
+   *
+   * Capabilities are a simple set of strings in the recipe. If the Normandy
+   * client has all of the capabilities listed, then execution continues. If not,
+   * `false` is returned.
+   *
+   * If the capabilities check passes, then the filter expression is evaluated
+   * against the current environment. The result of the expression is cast to a
+   * boolean and returned.
+   *
    * @param {object} recipe
-   * @param {string} recipe.filter The expression to evaluate against the environment.
+   * @param {Array<string>} recipe.capabilities The list of capabilities
+   *                        required to evaluate this recipe.
+   * @param {string} recipe.filter_expression The expression to evaluate against the environment.
+   * @param {Set<String>} runnerCapabilities The capabilities provided by this runner.
    * @return {boolean} The result of evaluating the filter, cast to a bool, or false
    *                   if an error occurred during evaluation.
    */
-  async checkFilter(recipe) {
+  async shouldRunRecipe(recipe) {
+    const runnerCapabilities = this.getCapabilities();
+    if (Array.isArray(recipe.capabilities)) {
+      for (const recipeCapability of recipe.capabilities) {
+        if (!runnerCapabilities.has(recipeCapability)) {
+          log.debug(
+            `Recipe "${recipe.name}" requires unknown capabilities. ` +
+              `Recipe capabilities: ${JSON.stringify(recipe.capabilities)}. ` +
+              `Local runner capabilities: ${JSON.stringify(
+                Array.from(runnerCapabilities)
+              )}`
+          );
+          await Uptake.reportRecipe(
+            recipe,
+            Uptake.RECIPE_INCOMPATIBLE_COMPATIBILITIES
+          );
+          return false;
+        }
+      }
+    }
+
     const context = this.getFilterContext(recipe);
     let result;
     try {

toolkit/components/normandy/lib/Uptake.jsm

@@ -30,6 +30,7 @@ var Uptake = {
   // Per-recipe uptake
   RECIPE_ACTION_DISABLED: UptakeTelemetry.STATUS.CUSTOM_1_ERROR,
   RECIPE_DIDNT_MATCH_FILTER: UptakeTelemetry.STATUS.BACKOFF,
+  RECIPE_INCOMPATIBLE_CAPABILITIES: UptakeTelemetry.STATUS.BACKOFF,
   RECIPE_EXECUTION_ERROR: UptakeTelemetry.STATUS.APPLY_ERROR,
   RECIPE_FILTER_BROKEN: UptakeTelemetry.STATUS.CONTENT_ERROR,
   RECIPE_INVALID_ACTION: UptakeTelemetry.STATUS.DOWNLOAD_ERROR,

toolkit/components/normandy/test/browser/browser_RecipeRunner.js

@@ -12,6 +12,10 @@ ChromeUtils.import("resource://normandy/lib/NormandyApi.jsm", this);
 ChromeUtils.import("resource://normandy/lib/ActionsManager.jsm", this);
 ChromeUtils.import("resource://normandy/lib/AddonStudies.jsm", this);
 ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this);
+ChromeUtils.import(
+  "resource://gre/modules/components-utils/FilterExpressions.jsm",
+  this
+);
 
 const { RemoteSettings } = ChromeUtils.import(
   "resource://services-settings/remote-settings.js"
@@ -74,9 +78,9 @@ add_task(async function getFilterContext() {
   is(context.env.userId, "some id", "userId was cached");
 });
 
-add_task(async function checkFilter() {
+add_task(async function test_shouldRunRecipe_filterExpressions() {
   const check = filter =>
-    RecipeRunner.checkFilter({ filter_expression: filter });
+    RecipeRunner.shouldRunRecipe({ filter_expression: filter });
 
   // Errors must result in a false return value.
   ok(
@@ -90,12 +94,12 @@ add_task(async function checkFilter() {
   // The given recipe must be available to the filter context.
   const recipe = { filter_expression: "normandy.recipe.id == 7", id: 7 };
   ok(
-    await RecipeRunner.checkFilter(recipe),
+    await RecipeRunner.shouldRunRecipe(recipe),
     "The recipe is available in the filter context"
   );
   recipe.id = 4;
   ok(
-    !(await RecipeRunner.checkFilter(recipe)),
+    !(await RecipeRunner.shouldRunRecipe(recipe)),
     "The recipe is available in the filter context"
   );
 });
@@ -103,14 +107,17 @@ add_task(async function checkFilter() {
 decorate_task(
   withStub(FilterExpressions, "eval"),
   withStub(Uptake, "reportRecipe"),
-  async function checkFilterCanHandleExceptions(evalStub, reportRecipeStub) {
+  async function test_shouldRunRecipe_canHandleExceptions(
+    evalStub,
+    reportRecipeStub
+  ) {
     evalStub.throws("this filter was broken somehow");
     const someRecipe = {
       id: "1",
       action: "action",
       filter_expression: "broken",
     };
-    const result = await RecipeRunner.checkFilter(someRecipe);
+    const result = await RecipeRunner.shouldRunRecipe(someRecipe);
 
     Assert.deepEqual(result, false, "broken filters are treated as false");
     Assert.deepEqual(reportRecipeStub.args, [
@@ -119,6 +126,47 @@ decorate_task(
   }
 );
 
+decorate_task(
+  withSpy(FilterExpressions, "eval"),
+  withStub(RecipeRunner, "getCapabilities"),
+  async function test_shouldRunRecipe_checksCapabilities(
+    evalSpy,
+    getCapabilitiesStub
+  ) {
+    getCapabilitiesStub.returns(new Set(["test-capability"]));
+
+    let result = await RecipeRunner.shouldRunRecipe({
+      filter_expression: "true",
+    });
+    ok(result, "Recipes with no capabilities should pass");
+    ok(evalSpy.called, "Filter should be evaluated");
+
+    evalSpy.resetHistory();
+    result = await RecipeRunner.shouldRunRecipe({
+      capabilities: [],
+      filter_expression: "true",
+    });
+    ok(result, "Recipes with empty capabilities should pass");
+    ok(evalSpy.called, "Filter should be evaluated");
+
+    evalSpy.resetHistory();
+    result = await RecipeRunner.shouldRunRecipe({
+      capabilities: ["test-capability"],
+      filter_expression: "true",
+    });
+    ok(result, "Recipes with a matching capability should pass");
+    ok(evalSpy.called, "Filter should be evaluated");
+
+    evalSpy.resetHistory();
+    result = await RecipeRunner.shouldRunRecipe({
+      capabilities: ["impossible-capability"],
+      filter_expression: "true",
+    });
+    ok(!result, "Recipes with non-matching capabilities should not pass");
+    ok(!evalSpy.called, "Filter should not be evaluated");
+  }
+);
+
 decorate_task(
   withMockNormandyApi,
   withStub(ClientEnvironment, "getClientClassification"),
@@ -236,16 +284,41 @@ decorate_task(
   }
 );
 
+decorate_task(
+  withPrefEnv({
+    set: [["features.normandy-remote-settings.enabled", true]],
+  }),
+  withStub(RecipeRunner, "getCapabilities"),
+  async function test_run_includesCapabilities(getCapabilitiesStub) {
+    const rsCollection = await RecipeRunner._remoteSettingsClientForTesting.openCollection();
+    await rsCollection.clear();
+    const fakeSig = { signature: "abc" };
+    await rsCollection.create(
+      { id: "match", recipe: { id: 1 }, signature: fakeSig },
+      { synced: true }
+    );
+    await rsCollection.db.saveLastModified(42);
+    rsCollection.db.close();
+
+    let capabilities = new Set(["test-capability"]);
+    getCapabilitiesStub.returns(capabilities);
+    await RecipeRunner.run();
+    ok(getCapabilitiesStub.called, "getCapabilities should be called");
+  }
+);
+
 decorate_task(
   withPrefEnv({
     set: [["features.normandy-remote-settings.enabled", true]],
   }),
   withStub(NormandyApi, "verifyObjectSignature"),
+  withSpy(NormandyApi, "fetchRecipes"),
   withStub(ActionsManager.prototype, "runRecipe"),
   withStub(ActionsManager.prototype, "finalize"),
   withStub(Uptake, "reportRecipe"),
   async function testReadFromRemoteSettings(
     verifyObjectSignatureStub,
+    fetchRecipesSpy,
     runRecipeStub,
     finalizeStub,
     reportRecipeStub
@@ -289,7 +362,7 @@ decorate_task(
     Assert.deepEqual(
       verifyObjectSignatureStub.args,
       [[matchRecipe, fakeSig, "recipe"], [missingRecipe, fakeSig, "recipe"]],
-      "recipes with matching filters should have their signature verified"
+      "recipes with matching should have their signature verified"
     );
     Assert.deepEqual(
       runRecipeStub.args,
@@ -301,6 +374,56 @@ decorate_task(
       [[noMatchRecipe, Uptake.RECIPE_DIDNT_MATCH_FILTER]],
       "Filtered-out recipes should be reported"
     );
+
+    ok(fetchRecipesSpy.notCalled, "fetchRecipes should not be called");
+  }
+);
+
+decorate_task(
+  withPrefEnv({
+    set: [["features.normandy-remote-settings.enabled", true]],
+  }),
+  withStub(NormandyApi, "verifyObjectSignature"),
+  withStub(ActionsManager.prototype, "runRecipe"),
+  withStub(RecipeRunner, "getCapabilities"),
+  async function testReadFromRemoteSettings(
+    verifyObjectSignatureStub,
+    runRecipeStub,
+    getCapabilitiesStub
+  ) {
+    getCapabilitiesStub.returns(new Set(["compatible"]));
+    const compatibleRecipe = {
+      name: "match",
+      filter_expression: "true",
+      capabilities: ["compatible"],
+    };
+    const incompatibleRecipe = {
+      name: "noMatch",
+      filter_expression: "true",
+      capabilities: ["incompatible"],
+    };
+
+    const rsCollection = await RecipeRunner._remoteSettingsClientForTesting.openCollection();
+    await rsCollection.clear();
+    const fakeSig = { signature: "abc" };
+    await rsCollection.create(
+      { id: "match", recipe: compatibleRecipe, signature: fakeSig },
+      { synced: true }
+    );
+    await rsCollection.create(
+      { id: "noMatch", recipe: incompatibleRecipe, signature: fakeSig },
+      { synced: true }
+    );
+    await rsCollection.db.saveLastModified(42);
+    rsCollection.db.close();
+
+    await RecipeRunner.run();
+
+    Assert.deepEqual(
+      runRecipeStub.args,
+      [[compatibleRecipe]],
+      "only recipes with compatible capabilities should be executed"
+    );
   }
 );
 

toolkit/components/utils/FilterExpressions.jsm

@@ -47,6 +47,10 @@ XPCOMUtils.defineLazyGetter(this, "jexl", () => {
 });
 
 var FilterExpressions = {
+  getAvailableTransforms() {
+    return Object.keys(jexl._transforms);
+  },
+
   eval(expr, context = {}) {
     const onelineExpr = expr.replace(/[\t\n\r]/g, " ");
     return jexl.eval(onelineExpr, context);