- changed name of the new option (from enforcePrivateVariables to mas…

…kVariableValues), but this is still TBD - use the same helper for both the deprecated privateVariable option and the new option, since the logic is (basically) the same - added helper (and tests) to enforce that originalVariables.keys == modifiedVariables.keys - updated documentation
apollographql · Jun 19, 2019 · 1ebc472 · 1ebc472
1 parent c8c99e4
commit 1ebc472
Show file tree

Hide file tree

Showing 5 changed files with 316 additions and 42 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,7 @@
 # Changelog
 
 ### vNext
+- `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Adding the new EngineReportingOption `maskVariableValues` to send some or all variable values, possibly after transforming them. This replaces the `privateVariables` option, which is now deprecated. [PR #2847](https://github.com/apollographql/apollo-server/pull/2847)
 
 ### v2.6.2
 

diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md
@@ -119,7 +119,7 @@ new ApolloServer({
 
 * `engine`: <`EngineReportingOptions`> | boolean
 
-  Provided the `ENGINE_API_KEY` environment variable is set, the engine reporting agent will be started automatically. The API key can also be provided as the `apiKey` field in an object passed as the `engine` field. See the [EngineReportingOptions](#enginereportingoptions) section for a full description of how to configure the reporting agent, including how to blacklist variables. When using the Engine proxy, this option should be set to false.
+  Provided the `ENGINE_API_KEY` environment variable is set, the engine reporting agent will be started automatically. The API key can also be provided as the `apiKey` field in an object passed as the `engine` field. See the [EngineReportingOptions](#enginereportingoptions) section for a full description of how to configure the reporting agent, including how to blocklist variables. When using the Engine proxy, this option should be set to false.
 
 * `persistedQueries`: <`Object`> | false
 
@@ -343,14 +343,31 @@ addMockFunctionsToSchema({
    to standard error. Specify this function to process errors in a different
    way.
 
-*  `privateVariables`: Array<String> | boolean
+*  `privateVariables`: Array<String\> | boolean
 
+   DEPRECATING IN VERSION XX.XX.XX for `maskVariableValues`, which will support the same
+   functionalities but allow for more flexibility.
+
    A case-sensitive list of names of variables whose values should not be sent
    to Apollo servers, or 'true' to leave out all variables. In the former
    case, the report will indicate that each private variable was redacted in
    the latter case, no variables are sent at all.
-
-*  `privateHeaders`: Array<String> | boolean
+
+* `maskVariableValues`: { valueModifier: (options: { variables: Record<string, any>, operationString?: string } ) => Record<string, any> }
+                     | { privateVariableNames: Array<String\> }
+                     | { always: boolean }
+
+    By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms:
+
+    - { always: ... }: true to blocklist, or false to whitelist all variable values
+    - { valueModifier: ... }: a custom function for modifying variable values
+    - { privateVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers
+
+    Defaults to blocklisting all variable values if both this parameter and
+    the to-be-deprecated `privateVariables` are not set. The report will also
+    indicate each private variable redacted if set to { always: ... } or { privateVariableNames: ... }
+
+*  `privateHeaders`: Array<String\> | boolean
 
    A case-insensitive list of names of HTTP headers whose values should not be
    sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike

diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts
@@ -6,7 +6,11 @@ import {
 import { Trace } from 'apollo-engine-reporting-protobuf';
 import { graphql } from 'graphql';
 import { Request } from 'node-fetch';
-import { EngineReportingExtension } from '../extension';
+import {
+  EngineReportingExtension,
+  makeTraceDetails,
+  makeTraceDetailsLegacy,
+} from '../extension';
 import { InMemoryLRUCache } from 'apollo-server-caching';
 
 test('trace construction', async () => {
@@ -83,3 +87,138 @@ test('trace construction', async () => {
   requestDidEnd();
   // XXX actually write some tests
 });
+
+const variables: Record<string, any> = {
+  testing: 'testing',
+  t2: 2,
+};
+
+test('check variableJson output for privacyEnforcer boolean type', () => {
+  // Case 1: No keys/values in variables to be filtered/not filtered
+  const emptyOutput = new Trace.Details();
+  expect(makeTraceDetails({}, { always: true })).toEqual(emptyOutput);
+  expect(makeTraceDetails({}, { always: false })).toEqual(emptyOutput);
+
+  // Case 2: Filter all variables
+  const filteredOutput = new Trace.Details();
+  Object.keys(variables).forEach(name => {
+    filteredOutput.variablesJson[name] = '';
+  });
+  expect(makeTraceDetails(variables, { always: true })).toEqual(filteredOutput);
+
+  // Case 3: Do not filter variables
+  const nonFilteredOutput = new Trace.Details();
+  Object.keys(variables).forEach(name => {
+    nonFilteredOutput.variablesJson[name] = JSON.stringify(variables[name]);
+  });
+  expect(makeTraceDetails(variables, { always: false })).toEqual(
+    nonFilteredOutput,
+  );
+});
+
+test('variableJson output for privacyEnforcer Array type', () => {
+  const privateVariablesArray: string[] = ['testing', 'notInVariables'];
+  const expectedVariablesJson = {
+    testing: '',
+    t2: JSON.stringify(2),
+  };
+  expect(
+    makeTraceDetails(variables, { privateVariableNames: privateVariablesArray })
+      .variablesJson,
+  ).toEqual(expectedVariablesJson);
+});
+
+test('variableJson output for privacyEnforcer custom function', () => {
+  // Custom function that redacts every variable to 100;
+  const modifiedValue = 100;
+  const customModifier = (input: {
+    variables: Record<string, any>;
+  }): Record<string, any> => {
+    let out: Record<string, any> = {};
+    Object.keys(input.variables).map((name: string) => {
+      out[name] = modifiedValue;
+    });
+    return out;
+  };
+
+  // Expected output
+  const output = new Trace.Details();
+  Object.keys(variables).forEach(name => {
+    output.variablesJson[name] = JSON.stringify(modifiedValue);
+  });
+
+  expect(
+    makeTraceDetails(variables, { valueModifier: customModifier }),
+  ).toEqual(output);
+});
+
+test('privacyEnforcer=True equivalent to privacyEnforcer=Array(all variables)', () => {
+  let privateVariablesArray: string[] = ['testing', 't2'];
+  expect(makeTraceDetails(variables, { always: true }).variablesJson).toEqual(
+    makeTraceDetails(variables, { privateVariableNames: privateVariablesArray })
+      .variablesJson,
+  );
+});
+
+test('original keys in variables match the modified keys', () => {
+  const origKeys = Object.keys(variables);
+  const modifiedKeys = Object.keys(variables).slice(1);
+  modifiedKeys.push('new v');
+
+  const modifier = (input: {
+    variables: Record<string, any>;
+  }): Record<string, any> => {
+    let out: Record<string, any> = {};
+    Object.keys(modifiedKeys).map((name: string) => {
+      out[name] = 100;
+    });
+    return out;
+  };
+
+  expect(
+    Object.keys(
+      makeTraceDetails(variables, { valueModifier: modifier }).variablesJson,
+    ).sort(),
+  ).toEqual(origKeys.sort());
+});
+
+/**
+ * Tests for old privateVariables reporting option
+ */
+
+test('test variableJson output for to-be-deprecated privateVariable option', () => {
+  // Case 1: privateVariables == False; same as makeTraceDetails
+  expect(makeTraceDetailsLegacy({}, false)).toEqual(
+    makeTraceDetails({}, { always: false }),
+  );
+  expect(makeTraceDetailsLegacy(variables, false)).toEqual(
+    makeTraceDetails(variables, { always: false }),
+  );
+
+  // Case 2: privateVariables is an Array; same as makeTraceDetails
+  const privacyEnforcerArray: string[] = ['testing', 'notInVariables'];
+  expect(
+    makeTraceDetailsLegacy(variables, privacyEnforcerArray).variablesJson,
+  ).toEqual(
+    makeTraceDetails(variables, { privateVariableNames: privacyEnforcerArray })
+      .variablesJson,
+  );
+});
+
+test('original keys in variables match the modified keys', () => {
+  const origKeys = Object.keys(variables);
+  const modifiedKeys = Object.keys(variables).slice(1);
+  modifiedKeys.push('new v');
+
+  const enforcer = (input: Record<string, any>): Record<string, any> => {
+    let out: Record<string, any> = {};
+    Object.keys(modifiedKeys).map((name: string) => {
+      out[name] = 100;
+    });
+    return out;
+  };
+
+  expect(
+    Object.keys(makeTraceDetails(variables, enforcer).variablesJson).sort(),
+  ).toEqual(origKeys.sort());
+});
diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts
@@ -25,6 +25,11 @@ export interface ClientInfo {
   clientReferenceId?: string;
 }
 
+export type VariableValueModifierOptions = {
+  variables: Record<string, any>;
+  operationString?: string;
+};
+
 export type GenerateClientInfo<TContext> = (
   requestContext: GraphQLRequestContext<TContext>,
 ) => ClientInfo;
@@ -82,12 +87,34 @@ export interface EngineReportingOptions<TContext> {
    */
   reportErrorFunction?: (err: Error) => void;
   /**
-   * A case-sensitive list of names of variables whose values should not be sent
-   * to Apollo servers, or 'true' to leave out all variables. In the former
+   * [TO-BE-DEPRECATED] A case-sensitive list of names of variables whose values should
+   * not be sent to Apollo servers, or 'true' to leave out all variables. In the former
    * case, the report will indicate that each private variable was redacted; in
    * the latter case, no variables are sent at all.
    */
   privateVariables?: Array<String> | boolean;
+  /**
+   * By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms:
+   * - true or false to blocklist or whitelist all variable values
+   * - a custom function for modifying variable values
+   * - a case-sensitive list of names of variables whose values should not be sent to Apollo servers
+   *
+   * Will default to blocklisting all variable values if both this parameter and
+   * the to-be-deprecated `privateVariables` were not set. The report will also
+   * indicate each private variable redacted if set to a boolean or array.
+   *
+   * TODO(helen): Add new flag to the trace details (and modify the protobuf message structure) to indicate the type of modification. Then, add the following description to the docs:
+   * "The report will indicate that variable values were modified by a custom function, or will list all private variables redacted."
+   * TODO(helen): LINK TO EXAMPLE FUNCTION? e.g. a function recursively search for keys to be blocklisted
+   */
+  maskVariableValues?:
+    | {
+        valueModifier: (
+          options: VariableValueModifierOptions,
+        ) => Record<string, any>;
+      }
+    | { privateVariableNames: Array<String> }
+    | { always: boolean };
   /**
    * A case-insensitive list of names of HTTP headers whose values should not be
    * sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike