(core) refactor assertCanMaybeApplyUserActions

Summary: This refactors assertCanMaybeApplyUserActions for clarity.

Test Plan: existing tests pass, added test

Reviewers: dsagal, jarek

Reviewed By: dsagal, jarek

Subscribers: jarek

Differential Revision: https://phab.getgrist.com/D3637

app/server/lib/ActiveDoc.ts

@@ -1791,7 +1791,7 @@ export class ActiveDoc extends EventEmitter {
     if (options?.bestEffort) {
       actions = await this._granularAccess.prefilterUserActions(docSession, actions);
     }
-    await this._granularAccess.assertCanMaybeApplyUserActions(docSession, actions);
+    await this._granularAccess.checkUserActions(docSession, actions);
 
     // Create the UserActionBundle.
     const action: UserActionBundle = {

app/server/lib/GranularAccess.ts

@@ -86,19 +86,20 @@ const STRUCTURAL_TABLES = new Set(['_grist_Tables', '_grist_Tables_column', '_gr
                                    '_grist_ACLResources', '_grist_ACLRules']);
 
 // Actions that won't be allowed (yet) for a user with nuanced access to a document.
-// A few may be innocuous, but generally I've put them in this list if there are problems
+// A few may be innocuous, but that hasn't been figured out yet.
-// tracking down what table the refer to, or they could allow creation/modification of a
-// formula, and are not handled elsewhere.
 const SPECIAL_ACTIONS = new Set(['InitNewDoc',
                                  'EvalCode',
                                  'UpdateSummaryViewSection',
                                  'DetachSummaryViewSection',
                                  'GenImporterView',
+                                 'MakeImportTransformColumns',
+                                 'FillTransformRuleColIds',
                                  'TransformAndFinishImport',
                                  'AddView',
                                  'CopyFromColumn',
                                  'ConvertFromColumn',
                                  'AddHiddenColumn',
+                                 'RespondToRequests',
                                 ]);
 
 // Odd-ball actions marked as deprecated or which seem unlikely to be used.
@@ -110,6 +111,55 @@ const SURPRISING_ACTIONS = new Set([
 // Actions we'll allow unconditionally for now.
 const OK_ACTIONS = new Set(['Calculate', 'UpdateCurrentTime']);
 
+// Other actions that are believed to be compatible with granular access.
+// Only add an action to OTHER_RECOGNIZED_ACTIONS if you know access control
+// has been handled for it, or it is clear that access control can be done
+// by looking at the Create/Update/Delete permissions for the DocActions it
+// will create. For example, at the time of writing CopyFromColumn should
+// not be here, since it could read a column the user is not supposed to
+// have access rights to, and it is not handled specially.
+const OTHER_RECOGNIZED_ACTIONS = new Set([
+  // Data actions.
+  'AddRecord',
+  'BulkAddRecord',
+  'UpdateRecord',
+  'BulkUpdateRecord',
+  'RemoveRecord',
+  'BulkRemoveRecord',
+  'ReplaceTableData',
+
+  // A data action handled specially because of read needs.
+  'AddOrUpdateRecord',
+
+  // Groups of actions.
+  'ApplyDocActions',
+  'ApplyUndoActions',
+
+  // Column-level schema changes.
+  'AddColumn',
+  'AddVisibleColumn',
+  'RemoveColumn',
+  'RenameColumn',
+  'ModifyColumn',
+
+  // Table-level schema changes.
+  'AddEmptyTable',
+  'AddTable',
+  'AddRawTable',
+  'RemoveTable',
+  'RenameTable',
+
+  // Display column support.
+  'SetDisplayFormula',
+  'MaybeCopyDisplayFormula',
+
+  // Sundry misc.
+  'RenameChoices',
+  'AddEmptyRule',
+  'CreateViewSection',
+  'RemoveViewSection',
+]);
+
 interface DocUpdateMessage {
   actionGroup: ActionGroup;
   docActions: DocAction[];
@@ -476,26 +526,24 @@ export class GranularAccess implements GranularAccessForBundle {
   }
 
   /**
-   * Check if user may be able to apply a list of actions.  Throws if
+   * Check the list of UserActions, throwing if something not permitted is found.
-   * user cannot apply an action.  Returns true if a user can perhaps apply an
+   * The data engine is the definitive interpreter of UserActions, but we do what
-   * action, or false if we know we need to defer making that determination
+   * we can, and then rely on analysis of DocActions produced by the data engine
-   * until the data engine translates the user actions to doc actions.
+   * later to finish the job. Any actions that read data and expose it in some way
+   * need to be caught at this point, since that won't be evident in the DocActions.
+   * So far, we've been restricting the permitted combinations of UserActions when
+   * data is read to make access control tractable. Likewise, any actions that might
+   * result in running user code that would not eventually be permitted needs to be
+   * caught now, since by the time it hits the data engine it is too late.
    */
-  public async assertCanMaybeApplyUserActions(docSession: OptDocSession, actions: UserAction[]): Promise<boolean> {
+  public async checkUserActions(docSession: OptDocSession, actions: UserAction[]): Promise<void> {
-    if (this._hasExceptionalFullAccess(docSession)) { return true; }
+    if (this._hasExceptionalFullAccess(docSession)) { return; }
-
-    let canMaybeApply = true;
-    for (const action of actions) {
-      if (!await this.assertCanMaybeApplyUserAction(docSession, action)) {
-        canMaybeApply = false;
-        break;
-      }
-    }
 
+    // Checks are in no particular order.
+    await this._checkSimpleDataActions(docSession, actions);
+    await this._checkForSpecialOrSurprisingActions(docSession, actions);
     await this._checkPossiblePythonFormulaModification(docSession, actions);
     await this._checkAddOrUpdateAccess(docSession, actions);
-
-    return canMaybeApply;
   }
 
   /**
@@ -581,50 +629,6 @@ export class GranularAccess implements GranularAccessForBundle {
     return [['ApplyUndoActions', proposedActions]];
   }
 
-  /**
-   * Check if user may be able to apply a given action.  Throws if
-   * user cannot apply the action.  Returns true if a user can apply an
-   * action, or false if we need to defer making that determination
-   * until the data engine translates the user actions to doc actions.
-   */
-  public async assertCanMaybeApplyUserAction(docSession: OptDocSession, a: UserAction|DocAction): Promise<boolean> {
-    const name = a[0] as string;
-    if (this._hasExceptionalFullAccess(docSession)) { return true; }
-    if (OK_ACTIONS.has(name)) { return true; }
-    if (SPECIAL_ACTIONS.has(name)) {
-      if (await this.hasNuancedAccess(docSession)) {
-        throw new ErrorWithCode('ACL_DENY', `Blocked by access rules: '${name}' actions need uncomplicated access`);
-      }
-      return true;
-    }
-    if (SURPRISING_ACTIONS.has(name)) {
-      if (!await this.hasFullAccess(docSession)) {
-        throw new ErrorWithCode('ACL_DENY', `Blocked by access rules: '${name}' actions need full access`);
-      }
-      return true;
-    }
-    if (name === 'ApplyUndoActions') {
-      return this.assertCanMaybeApplyUserActions(docSession, a[1] as UserAction[]);
-    } else if (name === 'ApplyDocActions') {
-      return this.assertCanMaybeApplyUserActions(docSession, a[1] as UserAction[]);
-    } else if (name === 'AddOrUpdateRecord') {
-      // This case is a bit tricky.
-      // Access is checked separately in _checkAddOrUpdateAccess.
-      return true;
-    } else if (isDataAction(a)) {
-      const tableId = getTableId(a);
-      if (tableId.startsWith('_grist_')) {
-        return false;  // have to look closely
-      }
-      const tableAccess = await this.getTableAccess(docSession, tableId);
-      const accessCheck = await this._getAccessForActionType(docSession, a, 'fatal');
-      accessCheck.get(tableAccess);  // will throw if access denied.
-      return true;
-    } else {
-      return false;  // have to look closely
-    }
-  }
-
   /**
    * For changes that could include Python formulas, check for schema access early.
    */
@@ -884,6 +888,79 @@ export class GranularAccess implements GranularAccessForBundle {
     return baseAccess;
   }
 
+  /**
+   * An optimization to catch obvious access problems for simple data
+   * actions (such as UpdateRecord, BulkAddRecord, etc) early. Checks
+   * actions one by one (nesting into ApplyUndoActions and
+   * ApplyDocActions as needed) until meeting one that isn't a simple
+   * data action. Checks are crude, and limited to the table access
+   * level. Returns true if all actions were checked, false if
+   * not. Returning true does not imply the actions in the bundle are
+   * permissible; returning false does not imply they should be
+   * denied. Throwing an error DOES imply that an action was
+   * encountered that should be denied.
+   */
+  private async _checkSimpleDataActions(docSession: OptDocSession, actions: UserAction[]): Promise<boolean> {
+    for (const action of actions) {
+      if (!await this._checkSimpleDataAction(docSession, action)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  /**
+   * Throws an error for simple data actions that the user cannot perform.
+   * Checking is only at the table level. Returns true if the action clearly
+   * does not change the document schema or metadata, otherwise false if it might.
+   */
+  private async _checkSimpleDataAction(docSession: OptDocSession, a: UserAction|DocAction): Promise<boolean> {
+    const name = a[0] as string;
+    if (name === 'ApplyUndoActions') {
+      return this._checkSimpleDataActions(docSession, a[1] as UserAction[]);
+    } else if (name === 'ApplyDocActions') {
+      return this._checkSimpleDataActions(docSession, a[1] as UserAction[]);
+    } else if (isDataAction(a)) {
+      const tableId = getTableId(a);
+      if (tableId.startsWith('_grist_')) {
+        return false;
+      }
+      const tableAccess = await this.getTableAccess(docSession, tableId);
+      const accessCheck = await this._getAccessForActionType(docSession, a, 'fatal');
+      accessCheck.get(tableAccess);  // will throw if access denied.
+      return true;
+    } else {
+      // Any other action might change schema, so continuing could lead
+      // to false detections of failures. For example, renaming a column
+      // and then updating cells within it should be allowed.
+      return false;
+    }
+  }
+
+  private async _checkForSpecialOrSurprisingActions(docSession: OptDocSession,
+                                                    actions: UserAction[]) {
+    await applyToActionsRecursively(actions, async (a) => {
+      const name = String(a[0]);
+      if (SPECIAL_ACTIONS.has(name)) {
+        if (await this.hasNuancedAccess(docSession)) {
+          throw new ErrorWithCode('ACL_DENY', `Blocked by access rules: '${name}' actions need uncomplicated access`);
+        }
+      } else if (SURPRISING_ACTIONS.has(name)) {
+        if (!await this.hasFullAccess(docSession)) {
+          throw new ErrorWithCode('ACL_DENY', `Blocked by access rules: '${name}' actions need full access`);
+        }
+      } else if (OK_ACTIONS.has(name)) {
+        // fine, anyone can do these at any time, continue.
+      } else if (OTHER_RECOGNIZED_ACTIONS.has(name)) {
+        // these are known actions that have not been specifically classified.
+      } else {
+        // we've hit something unexpected - perhaps a UserAction has been added
+        // without considering access control.
+        throw new ErrorWithCode('ACL_DENY', `Blocked by access rules: '${name}' actions are not controlled`);
+      }
+    });
+  }
+
   // AddOrUpdateRecord requires broad read access to a table.
   // But tables can be renamed, and access can be granted and removed
   // within a bundle.