feat(ng-dev/pullapprove): handle references to author in condition

* feat(ng-dev/pullapprove): handle references to `author` in condition

Necessary change for: angular/angular#44866

ng-dev/pullapprove/BUILD.bazel

@@ -4,6 +4,7 @@ ts_library(
     name = "pullapprove",
     srcs = [
         "cli.ts",
+        "condition_errors.ts",
         "condition_evaluator.ts",
         "group.ts",
         "logging.ts",

ng-dev/pullapprove/condition_errors.ts

@@ -0,0 +1,11 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+export class PullApproveGroupStateDependencyError extends Error {}
+
+export class PullApproveAuthorStateDependencyError extends Error {}

ng-dev/pullapprove/condition_evaluator.ts

@@ -6,23 +6,39 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {PullApproveGroup} from './group';
 import {PullApproveGroupArray, PullApproveStringArray} from './pullapprove_arrays';
+
+import {PullApproveAuthorStateDependencyError} from './condition_errors';
+import {PullApproveGroup} from './group';
 import {getOrCreateGlob} from './utils';
+import {runInNewContext} from 'vm';
 
 /**
- * Context that is provided to conditions. Conditions can use various helpers
- * that PullApprove provides. We try to mock them here. Consult the official
- * docs for more details: https://docs.pullapprove.com/config/conditions.
+ * Context object that will be used as global context in condition evaluation.
+ *
+ * Conditions can use various helpers that PullApprove provides. We try to
+ * mock them here. Consult the official docs for more details:
+ * https://docs.pullapprove.com/config/conditions.
  */
-const conditionContext = {
-  'len': (value: any[]) => value.length,
-  'contains_any_globs': (files: PullApproveStringArray, patterns: string[]) => {
-    // Note: Do not always create globs for the same pattern again. This method
-    // could be called for each source file. Creating glob's is expensive.
-    return files.some((f) => patterns.some((pattern) => getOrCreateGlob(pattern).match(f)));
-  },
-};
+const conditionEvaluationContext: object = (() => {
+  const context = {
+    'len': (value: any[]) => value.length,
+    'contains_any_globs': (files: PullApproveStringArray, patterns: string[]) => {
+      // Note: Do not always create globs for the same pattern again. This method
+      // could be called for each source file. Creating glob's is expensive.
+      return files.some((f) => patterns.some((pattern) => getOrCreateGlob(pattern).match(f)));
+    },
+  };
+
+  // We cannot process references to `author` in conditions.
+  Object.defineProperty(context, 'author', {
+    get: () => {
+      throw new PullApproveAuthorStateDependencyError();
+    },
+  });
+
+  return context;
+})();
 
 /**
  * Converts a given condition to a function that accepts a set of files. The returned
@@ -31,28 +47,19 @@ const conditionContext = {
 export function convertConditionToFunction(
   expr: string,
 ): (files: string[], groups: PullApproveGroup[]) => boolean {
-  // Creates a dynamic function with the specified expression.
-  // The first parameter will be `files` as that corresponds to the supported `files` variable that
-  // can be accessed in PullApprove condition expressions. The second parameter is the list of
-  // PullApproveGroups that are accessible in the condition expressions. The followed parameters
-  // correspond to other context variables provided by PullApprove for conditions.
-  const evaluateFn = new Function(
-    'files',
-    'groups',
-    ...Object.keys(conditionContext),
-    `
-    return (${transformExpressionToJs(expr)});
-  `,
-  );
+  const jsExpression = `
+    (files, groups) => {
+      return (${transformExpressionToJs(expr)});
+    }
+  `;
+  const isMatchingFn = runInNewContext(jsExpression, conditionEvaluationContext);
 
-  // Create a function that calls the dynamically constructed function which mimics
-  // the condition expression that is usually evaluated with Python in PullApprove.
   return (files, groups) => {
-    const result = evaluateFn(
+    const result = isMatchingFn(
       new PullApproveStringArray(...files),
       new PullApproveGroupArray(...groups),
-      ...Object.values(conditionContext),
     );
+
     // If an array is returned, we consider the condition as active if the array is not
     // empty. This matches PullApprove's condition evaluation that is based on Python.
     if (Array.isArray(result)) {

ng-dev/pullapprove/group.ts

@@ -6,10 +6,14 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {error} from '../utils/console';
-import {convertConditionToFunction} from './condition_evaluator';
+import {
+  PullApproveAuthorStateDependencyError,
+  PullApproveGroupStateDependencyError,
+} from './condition_errors';
+
 import {PullApproveGroupConfig} from './parse-yaml';
-import {PullApproveGroupStateDependencyError} from './pullapprove_arrays';
+import {convertConditionToFunction} from './condition_evaluator';
+import {error} from '../utils/console';
 
 /** A condition for a group. */
 interface GroupCondition {
@@ -34,9 +38,6 @@ export interface PullApproveGroupResult {
   unverifiableConditions: GroupCondition[];
 }
 
-// Regular expression that matches conditions for the global approval.
-const GLOBAL_APPROVAL_CONDITION_REGEX = /^"global-(docs-)?approvers" not in groups.approved$/;
-
 /** A PullApprove group to be able to test files against. */
 export class PullApproveGroup {
   /** List of conditions for the group. */
@@ -58,11 +59,6 @@ export class PullApproveGroup {
       return config.conditions.forEach((condition) => {
         const expression = condition.trim();
 
-        if (expression.match(GLOBAL_APPROVAL_CONDITION_REGEX)) {
-          // Currently a noop as we don't take any action for global approval conditions.
-          return;
-        }
-
         try {
           this.conditions.push({
             expression,
@@ -85,23 +81,31 @@ export class PullApproveGroup {
    * the pull approve group's conditions.
    */
   testFile(filePath: string): boolean {
-    return this.conditions.every((condition) => {
+    let allConditionsMet: boolean | null = null;
+
+    for (const condition of this.conditions) {
       const {matchedFiles, checkFn, expression} = condition;
       try {
         const matchesFile = checkFn([filePath], this.precedingGroups);
+
         if (matchesFile) {
           matchedFiles.add(filePath);
         }
-        return matchesFile;
+
+        allConditionsMet = (allConditionsMet ?? true) && matchesFile;
       } catch (e) {
-        // In the case of a condition that depends on the state of groups we want to
-        // ignore that the verification can't accurately evaluate the condition and then
-        // continue processing. Other types of errors fail the verification, as conditions
-        // should otherwise be able to execute without throwing.
-        if (e instanceof PullApproveGroupStateDependencyError) {
+        // If a group relies on the author state, we assume this group to never match
+        // or own a file. This is a strict assumption but prevents false-positives.
+        if (e instanceof PullApproveAuthorStateDependencyError) {
+          condition.unverifiable = true;
+          allConditionsMet = false;
+        }
+        // In the case of a condition that depends on the state of groups, we want to ignore
+        // that the verification can't accurately evaluate the condition and continue processing.
+        // Other types of errors fail the verification, as conditions should otherwise be able to
+        // execute without throwing.
+        else if (e instanceof PullApproveGroupStateDependencyError) {
           condition.unverifiable = true;
-          // Return true so that `this.conditions.every` can continue evaluating.
-          return true;
         } else {
           const errMessage =
             `Condition could not be evaluated: \n\n` +
@@ -111,7 +115,11 @@ export class PullApproveGroup {
           process.exit(1);
         }
       }
-    });
+    }
+
+    // A file matches the group when all conditions are met. A group is not considered
+    // as matching when all conditions have been skipped.
+    return allConditionsMet === true;
   }
 
   /** Retrieve the results for the Group, all matched and unmatched conditions. */

ng-dev/pullapprove/pullapprove_arrays.ts

@@ -5,11 +5,11 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
+
 import {PullApproveGroup} from './group';
+import {PullApproveGroupStateDependencyError} from './condition_errors';
 import {getOrCreateGlob} from './utils';
 
-export class PullApproveGroupStateDependencyError extends Error {}
-
 /**
  * Superset of a native array. The superset provides methods which mimic the
  * list data structure used in PullApprove for files in conditions.

ng-dev/pullapprove/verify.spec.ts

@@ -78,6 +78,20 @@ describe('group parsing', () => {
     const fwCore = getGroupByName(groups, 'fw-core')!;
     expect(() => fwCore.testFile('any')).not.toThrow();
   });
+
+  it('should never match groups with conditions that rely on author state', () => {
+    const groups = getGroupsFromYaml(`
+      groups:
+        renovate-notify-group:
+          conditions:
+            - author in ["renovate-bot"]
+            - contains_any_globs(files, ['packages/core/**'])
+      `);
+    const renovateGroup = getGroupByName(groups, 'renovate-notify-group')!;
+
+    expect(renovateGroup.testFile('packages/core/index.ts')).toBe(false);
+    expect(renovateGroup.conditions[0].unverifiable).toBe(true);
+  });
 });
 
 function getGroupByName(groups: PullApproveGroup[], name: string): PullApproveGroup | undefined {