(core) Checks that an ACL formula can be parsed, and prevent saving unparsable ACL rules.

Summary:
- Fix error-handling in bundleActions(), and wait for the full bundle to complete.
  (The omissions here were making it impossibly to react to errors from inside bundleActions())
- Catch problematic rules early enough to undo them, by trying out ruleCollection.update()
  on updated rules before the updates are applied.
- Added checkAclFormula() call to DocComm that checks parsing and compiling
  formula, and reports errors.
- In UI, prevent saving if any aclFormulas are invalid, or while waiting for the to get checked.

- Also fixed some lint errors

Test Plan: Added a test case of error reporting in ACL formulas.

Reviewers: paulfitz

Reviewed By: paulfitz

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

app/client/components/DocComm.ts

@@ -53,6 +53,7 @@ export class DocComm extends Disposable implements ActiveDocAPI {
   public reloadPlugins = this._wrapMethod("reloadPlugins");
   public reloadDoc = this._wrapMethod("reloadDoc");
   public fork = this._wrapMethod("fork");
+  public checkAclFormula = this._wrapMethod("checkAclFormula");
 
   public changeUrlIdEmitter = this.autoDispose(new Emitter());
 

app/client/models/DocData.ts

@@ -83,7 +83,7 @@ export class DocData extends BaseDocData {
     this._bundlesPending++;
 
     // Promise to allow waiting for the result of prepare() callback before it's even called.
-    let prepareResolve!: (value: T) => void;
+    let prepareResolve!: (value: T|Promise<T>) => void;
     const preparePromise = new Promise<T>(resolve => { prepareResolve = resolve; });
 
     // Manually-triggered promise for when finalize() should be called. It's triggered by user,
@@ -100,8 +100,7 @@ export class DocData extends BaseDocData {
         this._nextDesc = options.description;
         this._lastActionNum = null;
         this._triggerBundleFinalize = triggerFinalize;
-        const value = await options.prepare();
+        await prepareResolve(options.prepare());
-        prepareResolve(value);
         this._shouldIncludeInBundle = options.shouldIncludeInBundle;
 
         await triggerFinalizePromise;
@@ -123,8 +122,8 @@ export class DocData extends BaseDocData {
       }
     };
 
-    this._lastBundlePromise = doBundleActions();
+    const completionPromise = this._lastBundlePromise = doBundleActions();
-    return {preparePromise, triggerFinalize};
+    return {preparePromise, triggerFinalize, completionPromise};
   }
 
   // Execute a callback that may send multiple actions, and bundle those actions together. The
@@ -146,6 +145,7 @@ export class DocData extends BaseDocData {
       return await bundlingInfo.preparePromise;
     } finally {
       bundlingInfo.triggerFinalize();
+      await bundlingInfo.completionPromise;
     }
   }
 
@@ -261,7 +261,7 @@ export interface BundlingOptions<T = unknown> {
 
 /**
  * Result of startBundlingActions(), to allow waiting for prepare() to complete, and to trigger
- * finalize() manually.
+ * finalize() manually, and to wait for the full bundle to complete.
  */
 export interface BundlingInfo<T = unknown> {
   // Promise for when the prepare() has completed. Note that sometimes it's delayed until the
@@ -270,4 +270,7 @@ export interface BundlingInfo<T = unknown> {
 
   // Ask DocData to call the finalize callback immediately.
   triggerFinalize: () => void;
+
+  // Promise for when the bundle has been finalized.
+  completionPromise: Promise<void>;
 }

app/client/ui/AccessRules.ts

@@ -29,14 +29,23 @@ type RuleRec = Partial<SchemaTypes["_grist_ACLRules"]> & {id?: number, resourceR
 
 type UseCB = <T>(obs: BaseObservable<T>) => T;
 
+// Status of rules, which determines whether the "Save" button is enabled. The order of the values
+// matters, as we take the max of all the parts to determine the ultimate status.
+enum RuleStatus {
+  Unchanged,
+  ChangedValid,
+  Invalid,
+  CheckPending,
+}
+
 /**
  * Top-most container managing state and dom-building for the ACL rule UI.
  */
 export class AccessRules extends Disposable {
   // Whether anything has changed, i.e. whether to show a "Save" button.
-  public isAnythingChanged: Computed<boolean>;
+  private _ruleStatus: Computed<RuleStatus>;
 
-  // Parsed rules obtained from DocData during last call to update(). Used for isAnythingChanged.
+  // Parsed rules obtained from DocData during last call to update(). Used for _ruleStatus.
   private _ruleCollection = new ACLRuleCollection();
 
   // Array of all per-table rules.
@@ -51,20 +60,28 @@ export class AccessRules extends Disposable {
   // Array of all UserAttribute rules.
   private _userAttrRules = this.autoDispose(obsArray<ObsUserAttributeRule>());
 
+  // Whether the save button should be enabled.
+  private _savingEnabled: Computed<boolean>;
+
   constructor(private _gristDoc: GristDoc) {
     super();
-    this.isAnythingChanged = Computed.create(this, (use) => {
+    this._ruleStatus = Computed.create(this, (use) => {
       const defRuleSet = use(this._docDefaultRuleSet);
       const tableRules = use(this._tableRules);
       const userAttr = use(this._userAttrRules);
-      return (defRuleSet && use(defRuleSet.isChanged)) ||
+      return Math.max(
-        // If any table was changed or added, some t.isChanged will be set. If there were only
+        defRuleSet ? use(defRuleSet.ruleStatus) : RuleStatus.Unchanged,
-        // removals, then tableRules.length will have changed.
+        // If any tables/userAttrs were changed or added, they will be considered changed. If
-        tableRules.length !== this._ruleCollection.getAllTableIds().length ||
+        // there were only removals, then length will be reduced.
-        tableRules.some(t => use(t.isChanged)) ||
+        getChangedStatus(tableRules.length < this._ruleCollection.getAllTableIds().length),
-        userAttr.length !== this._ruleCollection.getUserAttributeRules().size ||
+        getChangedStatus(userAttr.length < this._ruleCollection.getUserAttributeRules().size),
-        userAttr.some(u => use(u.isChanged));
+        ...tableRules.map(t => use(t.ruleStatus)),
+        ...userAttr.map(u => use(u.ruleStatus)),
+      );
     });
+
+    this._savingEnabled = Computed.create(this, this._ruleStatus, (use, s) => (s === RuleStatus.ChangedValid));
+
     this.update().catch(reportError);
   }
 
@@ -78,7 +95,7 @@ export class AccessRules extends Disposable {
       rules.getAllTableIds().map(tableId => TableRules.create(this._tableRules,
           tableId, this, rules.getAllColumnRuleSets(tableId), rules.getTableDefaultRuleSet(tableId)))
     );
-    DefaultObsRuleSet.create(this._docDefaultRuleSet, null, undefined, rules.getDocDefaultRuleSet());
+    DefaultObsRuleSet.create(this._docDefaultRuleSet, this, null, undefined, rules.getDocDefaultRuleSet());
     this._userAttrRules.set(
       Array.from(rules.getUserAttributeRules().values(), userAttr =>
         ObsUserAttributeRule.create(this._userAttrRules, this, userAttr))
@@ -89,7 +106,7 @@ export class AccessRules extends Disposable {
    * Collect the internal state into records and sync them to the document.
    */
   public async save(): Promise<void> {
-    if (!this.isAnythingChanged.get()) { return; }
+    if (!this._savingEnabled.get()) { return; }
 
     // Note that if anything has changed, we apply changes relative to the current state of the
     // ACL tables (they may have changed by other users). So our changes will win.
@@ -169,6 +186,11 @@ export class AccessRules extends Disposable {
       }
       // Finally we can sync the records.
       await syncRecords(rulesTable, newRules);
+    }).catch(e => {
+      // Report the error, but go on to update the rules. The user may lose their entries, but
+      // will see what's in the document. To preserve entries and show what's wrong, we try to
+      // catch errors earlier.
+      reportError(e);
     });
 
     // Re-populate the state from DocData once the records are synced.
@@ -178,12 +200,19 @@ export class AccessRules extends Disposable {
   public buildDom() {
     return [
       cssAddTableRow(
-        bigBasicButton('Saved', {disabled: true}, dom.hide(this.isAnythingChanged)),
+        bigBasicButton({disabled: true}, dom.hide(this._savingEnabled),
-        bigPrimaryButton('Save', dom.show(this.isAnythingChanged),
+          dom.text((use) => {
+            const s = use(this._ruleStatus);
+            return s === RuleStatus.CheckPending ? 'Checking...' :
+              s === RuleStatus.Invalid ? 'Invalid' : 'Saved';
+          }),
+          testId('rules-non-save')
+        ),
+        bigPrimaryButton('Save', dom.show(this._savingEnabled),
           dom.on('click', () => this.save()),
           testId('rules-save'),
         ),
-        bigBasicButton('Revert', dom.show(this.isAnythingChanged),
+        bigBasicButton('Revert', dom.show(this._savingEnabled),
           dom.on('click', () => this.update()),
           testId('rules-revert'),
         ),
@@ -240,6 +269,12 @@ export class AccessRules extends Disposable {
     removeItem(this._userAttrRules, userAttr);
   }
 
+  public async checkAclFormula(text: string): Promise<void> {
+    if (text) {
+      return this._gristDoc.docComm.checkAclFormula(text);
+    }
+  }
+
   private _addTableRules(tableId: string) {
     if (this._tableRules.get().some(t => t.tableId === tableId)) {
       throw new Error(`Trying to add TableRules for existing table ${tableId}`);
@@ -255,8 +290,8 @@ export class AccessRules extends Disposable {
 
 // Represents all rules for a table.
 class TableRules extends Disposable {
-  // Whether any table rules changed. Always true if this._colRuleSets is undefined.
+  // Whether any table rules changed, and if they are valid.
-  public isChanged: Computed<boolean>;
+  public ruleStatus: Computed<RuleStatus>;
 
   // The column-specific rule sets.
   private _columnRuleSets = this.autoDispose(obsArray<ColumnObsRuleSet>());
@@ -267,29 +302,32 @@ class TableRules extends Disposable {
   // The default rule set (for columns '*'), if one is set.
   private _defaultRuleSet = Observable.create<DefaultObsRuleSet|null>(this, null);
 
-  constructor(public readonly tableId: string, private _accessRules: AccessRules,
+  constructor(public readonly tableId: string, public _accessRules: AccessRules,
               private _colRuleSets?: RuleSet[], private _defRuleSet?: RuleSet) {
     super();
     this._columnRuleSets.set(this._colRuleSets?.map(rs =>
-      ColumnObsRuleSet.create(this._columnRuleSets, this, rs, rs.colIds === '*' ? [] : rs.colIds)) || []);
+      ColumnObsRuleSet.create(this._columnRuleSets, this._accessRules, this, rs,
+        rs.colIds === '*' ? [] : rs.colIds)) || []);
 
     if (!this._colRuleSets) {
       // Must be a newly-created TableRules object. Just create a default RuleSet (for tableId:*)
-      DefaultObsRuleSet.create(this._defaultRuleSet, this, this._haveColumnRules);
+      DefaultObsRuleSet.create(this._defaultRuleSet, this._accessRules, this, this._haveColumnRules);
     } else if (this._defRuleSet) {
-      DefaultObsRuleSet.create(this._defaultRuleSet, this, this._haveColumnRules, this._defRuleSet);
+      DefaultObsRuleSet.create(this._defaultRuleSet, this._accessRules, this, this._haveColumnRules,
+        this._defRuleSet);
     }
 
-    this.isChanged = Computed.create(this, (use) => {
+    this.ruleStatus = Computed.create(this, (use) => {
-      if (!this._colRuleSets) { return true; }              // This TableRules object must be newly-added
       const columnRuleSets = use(this._columnRuleSets);
       const d = use(this._defaultRuleSet);
-      return (
+      return Math.max(
-        Boolean(d) !== Boolean(this._defRuleSet) ||         // Default rule set got added or removed
+        getChangedStatus(
-        (d && use(d.isChanged)) ||                          // Or changed
+          !this._colRuleSets ||                               // This TableRules object must be newly-added
-        columnRuleSets.length < this._colRuleSets.length || // There was a removal
+          Boolean(d) !== Boolean(this._defRuleSet) ||         // Default rule set got added or removed
-        columnRuleSets.some(rs => use(rs.isChanged))        // There was an addition or a change.
+          columnRuleSets.length < this._colRuleSets.length    // There was a removal
-      );
+        ),
+        d ? use(d.ruleStatus) : RuleStatus.Unchanged,         // Default rule set got changed.
+        ...columnRuleSets.map(rs => use(rs.ruleStatus)));     // Column rule set was added or changed.
     });
   }
 
@@ -364,12 +402,12 @@ class TableRules extends Disposable {
   }
 
   private _addColumnRuleSet() {
-    this._columnRuleSets.push(ColumnObsRuleSet.create(this._columnRuleSets, this, undefined, []));
+    this._columnRuleSets.push(ColumnObsRuleSet.create(this._columnRuleSets, this._accessRules, this, undefined, []));
   }
 
   private _addDefaultRuleSet() {
     if (!this._defaultRuleSet.get()) {
-      DefaultObsRuleSet.create(this._defaultRuleSet, this, this._haveColumnRules);
+      DefaultObsRuleSet.create(this._defaultRuleSet, this._accessRules, this, this._haveColumnRules);
     }
   }
 }
@@ -377,8 +415,8 @@ class TableRules extends Disposable {
 // Represents one RuleSet, for a combination of columns in one table, or the default RuleSet for
 // all remaining columns in a table.
 abstract class ObsRuleSet extends Disposable {
-  // Whether rules changed. Always true if this._ruleSet is undefined.
+  // Whether rules changed, and if they are valid. Never unchanged if this._ruleSet is undefined.
-  public isChanged: Computed<boolean>;
+  public ruleStatus: Computed<RuleStatus>;
 
   // Whether the rule set includes any conditions besides the default rule.
   public haveConditions: Computed<boolean>;
@@ -388,7 +426,7 @@ abstract class ObsRuleSet extends Disposable {
   private _body = this.autoDispose(obsArray<ObsRulePart>());
 
   // ruleSet is omitted for a new ObsRuleSet added by the user.
-  constructor(private _tableRules: TableRules|null, private _ruleSet?: RuleSet) {
+  constructor(public accessRules: AccessRules, private _tableRules: TableRules|null, private _ruleSet?: RuleSet) {
     super();
     if (this._ruleSet) {
       this._body.set(this._ruleSet.body.map(part => ObsRulePart.create(this._body, this, part)));
@@ -397,11 +435,12 @@ abstract class ObsRuleSet extends Disposable {
       this._body.set([ObsRulePart.create(this._body, this, undefined, true)]);
     }
 
-    this.isChanged = Computed.create(this, this._body, (use, body) => {
+    this.ruleStatus = Computed.create(this, this._body, (use, body) => {
-      // If anything was changed or added, some part.isChanged will be set. If there were only
+      // If anything was changed or added, some part.ruleStatus will be other than Unchanged. If
-      // removals, then body.length will have changed.
+      // there were only removals, then body.length will have changed.
-      return (body.length !== (this._ruleSet?.body?.length || 0) ||
+      return Math.max(
-              body.some(part => use(part.isChanged)));
+        getChangedStatus(body.length < (this._ruleSet?.body?.length || 0)),
+        ...body.map(part => use(part.ruleStatus)));
     });
 
     this.haveConditions = Computed.create(this, this._body, (use, body) => body.some(p => !p.isDefault));
@@ -469,11 +508,14 @@ class ColumnObsRuleSet extends ObsRuleSet {
   private _colIds = Observable.create<string[]>(this, this._initialColIds);
   private _colIdStr = Computed.create(this, (use) => use(this._colIds).join(", "));
 
-  constructor(tableRules: TableRules, ruleSet: RuleSet|undefined, private _initialColIds: string[]) {
+  constructor(accessRules: AccessRules, tableRules: TableRules, ruleSet: RuleSet|undefined,
-    super(tableRules, ruleSet);
+              private _initialColIds: string[]) {
-    const baseIsChanged = this.isChanged;
+    super(accessRules, tableRules, ruleSet);
-    this.isChanged = Computed.create(this, (use) =>
+    const baseRuleStatus = this.ruleStatus;
-      !isEqual(use(this._colIds), this._initialColIds) || use(baseIsChanged));
+    this.ruleStatus = Computed.create(this, (use) => Math.max(
+        getChangedStatus(!isEqual(use(this._colIds), this._initialColIds)),
+        use(baseRuleStatus)
+    ));
   }
 
   public buildDom() {
@@ -499,8 +541,9 @@ class ColumnObsRuleSet extends ObsRuleSet {
 }
 
 class DefaultObsRuleSet extends ObsRuleSet {
-  constructor(tableRules: TableRules|null, private _haveColumnRules?: Observable<boolean>, ruleSet?: RuleSet) {
+  constructor(accessRules: AccessRules, tableRules: TableRules|null,
-    super(tableRules, ruleSet);
+              private _haveColumnRules?: Observable<boolean>, ruleSet?: RuleSet) {
+    super(accessRules, tableRules, ruleSet);
   }
   public buildDom() {
     return cssRuleSet(
@@ -515,7 +558,7 @@ class DefaultObsRuleSet extends ObsRuleSet {
 }
 
 class ObsUserAttributeRule extends Disposable {
-  public isChanged: Computed<boolean>;
+  public ruleStatus: Computed<RuleStatus>;
 
   private _name = Observable.create<string>(this, this._userAttr?.name || '');
   private _tableId = Observable.create<string>(this, this._userAttr?.tableId || '');
@@ -524,12 +567,13 @@ class ObsUserAttributeRule extends Disposable {
 
   constructor(private _accessRules: AccessRules, private _userAttr?: UserAttributeRule) {
     super();
-    this.isChanged = Computed.create(this, use => (
+    this.ruleStatus = Computed.create(this, use =>
-      use(this._name) !== this._userAttr?.name ||
+      getChangedStatus(
-      use(this._tableId) !== this._userAttr?.tableId ||
+        use(this._name) !== this._userAttr?.name ||
-      use(this._lookupColId) !== this._userAttr?.lookupColId ||
+        use(this._tableId) !== this._userAttr?.tableId ||
-      use(this._charId) !== this._userAttr?.charId
+        use(this._lookupColId) !== this._userAttr?.lookupColId ||
-    ));
+        use(this._charId) !== this._userAttr?.charId
+      ));
   }
 
   public buildDom() {
@@ -570,8 +614,8 @@ class ObsUserAttributeRule extends Disposable {
 // Represents one line of a RuleSet, a combination of an aclFormula and permissions to apply to
 // requests that match it.
 class ObsRulePart extends Disposable {
-  // Whether rules changed. Always true if this._rulePart is undefined.
+  // Whether the rule part, and if it's valid or being checked.
-  public isChanged: Computed<boolean>;
+  public ruleStatus: Computed<RuleStatus>;
 
   // Formula to show in the "advanced" UI.
   private _aclFormula = Observable.create<string>(this, this._rulePart?.aclFormula || "");
@@ -582,13 +626,23 @@ class ObsRulePart extends Disposable {
 
   private _permissionsText = Computed.create(this, this._permissions, (use, p) => permissionSetToText(p));
 
+  // Whether the rule is being checked after a change. Saving will wait for such checks to finish.
+  private _checkPending = Observable.create(this, false);
+
+  // If the formula failed validation, the error message to show. Blank if valid.
+  private _formulaError = Observable.create(this, '');
+
   // rulePart is omitted for a new ObsRulePart added by the user.
   constructor(private _ruleSet: ObsRuleSet, private _rulePart?: RulePart,
               public readonly isDefault: boolean = (_rulePart?.aclFormula === '')) {
     super();
-    this.isChanged = Computed.create(this, (use) => {
+    this.ruleStatus = Computed.create(this, (use) => {
-      return (use(this._aclFormula) !== this._rulePart?.aclFormula ||
+      if (use(this._formulaError)) { return RuleStatus.Invalid; }
-        !isEqual(use(this._permissions), this._rulePart?.permissions));
+      if (use(this._checkPending)) { return RuleStatus.CheckPending; }
+      return getChangedStatus(
+        use(this._aclFormula) !== this._rulePart?.aclFormula ||
+        !isEqual(use(this._permissions), this._rulePart?.permissions)
+      );
     });
   }
 
@@ -612,17 +666,20 @@ class ObsRulePart extends Disposable {
           testId('rule-add'),
         )
       ),
-      cssConditionInput(
+      cssCondition(
-        this._aclFormula, async (text) => this._aclFormula.set(text),
+        cssConditionInput(
-        dom.prop('disabled', this.isBuiltIn()),
+          this._aclFormula, this._setAclFormula.bind(this),
-        dom.prop('placeholder', (use) => {
+          dom.prop('disabled', this.isBuiltIn()),
-          return (
+          dom.prop('placeholder', (use) => {
-            this._ruleSet.isSoleCondition(use, this) ? 'Everyone' :
+            return (
-            this._ruleSet.isLastCondition(use, this) ? 'Everyone Else' :
+              this._ruleSet.isSoleCondition(use, this) ? 'Everyone' :
-            'Enter Condition'
+              this._ruleSet.isLastCondition(use, this) ? 'Everyone Else' :
-          );
+              'Enter Condition'
-        }),
+            );
-        testId('rule-acl-formula'),
+          }),
+          testId('rule-acl-formula'),
+        ),
+        dom.maybe(this._formulaError, (msg) => cssConditionError(msg, testId('rule-error'))),
       ),
       cssPermissionsInput(
         this._permissionsText, async (p) => this._permissions.set(parsePermissions(p)),
@@ -647,6 +704,19 @@ class ObsRulePart extends Disposable {
   private _isNonFirstBuiltIn(): boolean {
     return this.isBuiltIn() && this._ruleSet.getFirstBuiltIn() !== this;
   }
+
+  private async _setAclFormula(text: string) {
+    this._aclFormula.set(text);
+    this._checkPending.set(true);
+    this._formulaError.set('');
+    try {
+      await this._ruleSet.accessRules.checkAclFormula(text);
+    } catch (e) {
+      this._formulaError.set(e.message);
+    } finally {
+      this._checkPending.set(false);
+    }
+  }
 }
 
 
@@ -760,6 +830,10 @@ function removeItem<T>(observableArray: MutableObsArray<T>, item: T): boolean {
   return false;
 }
 
+function getChangedStatus(value: boolean): RuleStatus {
+  return value ? RuleStatus.ChangedValid : RuleStatus.Unchanged;
+}
+
 const cssAddTableRow = styled('div', `
   margin: 16px 64px 0 64px;
   display: flex;
@@ -815,24 +889,28 @@ const cssRuleSetBody = styled('div', `
 
 const cssRulePart = styled('div', `
   display: flex;
-  align-items: center;
+  align-items: start;
   margin: 4px 0;
-  &-default {
+`);
-    margin-top: auto;
+
-  }
+const cssCondition = styled('div', `
+  min-width: 0;
+  flex: 1;
+  display: flex;
+  flex-direction: column;
 `);
 
 const cssConditionInput = styled(textInput, `
-  display: flex;
-  min-width: 0;
-  flex: 1;
-
   &[disabled] {
     background-color: ${colors.mediumGreyOpaque};
     color: ${colors.dark};
   }
 `);
 
+const cssConditionError = styled('div', `
+  color: ${colors.error};
+`);
+
 const cssPermissionsInput = styled(cssConditionInput, `
   margin-left: 8px;
   width: 64px;
@@ -843,6 +921,7 @@ const cssIconSpace = styled('div', `
   flex: none;
   height: 24px;
   width: 24px;
+  margin: 2px;
 `);
 
 const cssIconButton = styled(cssIconSpace, `

app/common/ACLRuleCollection.ts

@@ -49,6 +49,10 @@ const EMERGENCY_RULE_SET: RuleSet = {
 };
 
 export class ACLRuleCollection {
+  // Store error if one occurs while reading rules.  Rules are replaced with emergency rules
+  // in this case.
+  public ruleError: Error|undefined;
+
   // In the absence of rules, some checks are skipped. For now this is important to maintain all
   // existing behavior. TODO should make sure checking access against default rules is equivalent
   // and efficient.
@@ -72,10 +76,6 @@ export class ACLRuleCollection {
   // Maps name to the corresponding UserAttributeRule.
   private _userAttributeRules = new Map<string, UserAttributeRule>();
 
-  // Store error if one occurs while reading rules.  Rules are replaced with emergency rules
-  // in this case.
-  public ruleError: Error|undefined;
-
   // Whether there are ANY user-defined rules.
   public haveRules(): boolean {
     return this._haveRules;
@@ -170,7 +170,7 @@ export class ACLRuleCollection {
     try {
       this.ruleError = undefined;
       return readAclRules(docData, options);
-    } catch(e) {
+    } catch (e) {
       this.ruleError = e;  // Report the error indirectly.
       return {ruleSets: [EMERGENCY_RULE_SET], userAttributes: []};
     }

app/common/ActiveDocAPI.ts

@@ -222,4 +222,9 @@ export interface ActiveDocAPI {
    * Prepare a fork of the document, and return the id(s) of the fork.
    */
   fork(): Promise<ForkResult>;
+
+  /**
+   * Check if an ACL formula is valid. If not, will throw an error with an explanation.
+   */
+  checkAclFormula(text: string): Promise<void>;
 }

app/server/lib/ActiveDoc.ts

@@ -36,6 +36,7 @@ import {UploadResult} from 'app/common/uploads';
 import {DocReplacementOptions, DocState} from 'app/common/UserAPI';
 import {ParseOptions} from 'app/plugin/FileParserAPI';
 import {GristDocAPI} from 'app/plugin/GristAPI';
+import {compileAclFormula} from 'app/server/lib/ACLFormula';
 import {Authorizer} from 'app/server/lib/Authorizer';
 import {checksumFile} from 'app/server/lib/checksumFile';
 import {Client} from 'app/server/lib/Client';
@@ -898,6 +899,24 @@ export class ActiveDoc extends EventEmitter {
     return makeForkIds({userId, isAnonymous, trunkDocId, trunkUrlId});
   }
 
+  /**
+   * Check if an ACL formula is valid. If not, will throw an error with an explanation.
+   */
+  public async checkAclFormula(docSession: DocSession, text: string): Promise<void> {
+    // Checks can leak names of tables and columns.
+    if (!await this._granularAccess.canReadEverything(docSession)) { return; }
+    await this.waitForInitialization();
+    try {
+      const parsedAclFormula = await this._pyCall('parse_acl_formula', text);
+      compileAclFormula(parsedAclFormula);
+      // TODO We also need to check the validity of attributes, and of tables and columns
+      // mentioned in resources and userAttribute rules.
+    } catch (e) {
+      e.message = e.message?.replace('[Sandbox] ', '');
+      throw e;
+    }
+  }
+
   public getGristDocAPI(): GristDocAPI {
     return this.docPluginManager.gristDocAPI;
   }

app/server/lib/DocWorker.ts

@@ -110,6 +110,7 @@ export class DocWorker {
       getActionSummaries:       activeDocMethod.bind(null, 'viewers', 'getActionSummaries'),
       reloadDoc:                activeDocMethod.bind(null, 'editors', 'reloadDoc'),
       fork:                     activeDocMethod.bind(null, 'viewers', 'fork'),
+      checkAclFormula:          activeDocMethod.bind(null, 'viewers', 'checkAclFormula'),
     });
   }
 

app/server/lib/GranularAccess.ts

@@ -5,9 +5,11 @@ import { ACLRuleCollection } from 'app/common/ACLRuleCollection';
 import { ActionGroup } from 'app/common/ActionGroup';
 import { createEmptyActionSummary } from 'app/common/ActionSummary';
 import { Query } from 'app/common/ActiveDocAPI';
+import { ApiError } from 'app/common/ApiError';
 import { AsyncCreate } from 'app/common/AsyncCreate';
-import { AddRecord, BulkAddRecord, BulkColValues, BulkRemoveRecord, BulkUpdateRecord, CellValue,
+import { AddRecord, BulkAddRecord, BulkColValues, BulkRemoveRecord, BulkUpdateRecord } from 'app/common/DocActions';
-  ColValues, DocAction, getTableId, isSchemaAction, RemoveRecord, ReplaceTableData, UpdateRecord } from 'app/common/DocActions';
+import { RemoveRecord, ReplaceTableData, UpdateRecord } from 'app/common/DocActions';
+import { CellValue, ColValues, DocAction, getTableId, isSchemaAction } from 'app/common/DocActions';
 import { TableDataAction, UserAction } from 'app/common/DocActions';
 import { DocData } from 'app/common/DocData';
 import { ErrorWithCode } from 'app/common/ErrorWithCode';
@@ -109,7 +111,10 @@ export class GranularAccess {
   // Flag tracking whether a set of actions have been applied to the database or not.
   private _applied: boolean = false;
 
-  public constructor(private _docData: DocData, private _fetchQueryFromDB: (query: Query) => Promise<TableDataAction>, private _recoveryMode: boolean) {
+  public constructor(
+    private _docData: DocData,
+    private _fetchQueryFromDB: (query: Query) => Promise<TableDataAction>,
+    private _recoveryMode: boolean) {
   }
 
   /**
@@ -145,10 +150,32 @@ export class GranularAccess {
    */
   public async canApplyDocActions(docSession: OptDocSession, docActions: DocAction[], undo: DocAction[]) {
     this._applied = false;
-    if (!this._ruleCollection.haveRules()) { return; }
+    if (this._ruleCollection.haveRules()) {
-    this._prepareRowSnapshots(docActions, undo);
+      this._prepareRowSnapshots(docActions, undo);
-    await Promise.all(
+      await Promise.all(
-      docActions.map((action, idx) => this._checkIncomingDocAction(docSession, action, idx)));
+        docActions.map((action, idx) => this._checkIncomingDocAction(docSession, action, idx)));
+    }
+
+    // If the actions change any rules, verify that we'll be able to handle the changed rules. If
+    // they are to cause an error, reject the action to avoid forcing user into recovery mode.
+    if (docActions.some(docAction => ['_grist_ACLRules', '_grist_Resources'].includes(getTableId(docAction)))) {
+      // Create a tmpDocData with just the tables we care about, then update docActions to it.
+      const tmpDocData: DocData = new DocData(
+        (tableId) => { throw new Error("Unexpected DocData fetch"); }, {
+          _grist_ACLResources: this._docData.getTable('_grist_ACLResources')!.getTableDataAction(),
+          _grist_ACLRules: this._docData.getTable('_grist_ACLRules')!.getTableDataAction(),
+        });
+      for (const da of docActions) {
+        tmpDocData.receiveAction(da);
+      }
+
+      // Use the post-actions data to process the rules collection, and throw error if that fails.
+      const ruleCollection = new ACLRuleCollection();
+      await ruleCollection.update(tmpDocData, {log, compile: compileAclFormula});
+      if (ruleCollection.ruleError && !this._recoveryMode) {
+        throw new ApiError(ruleCollection.ruleError.message, 400);
+      }
+    }
   }
 
   /**
@@ -164,7 +191,8 @@ export class GranularAccess {
     this._applied = true;
     // If there is a rule change, redo from scratch for now.
     // TODO: this is placeholder code. Should deal with connected clients.
-    if (docActions.some(docAction => getTableId(docAction) === '_grist_ACLRules' || getTableId(docAction) === '_grist_Resources')) {
+    if (docActions.some(docAction => getTableId(docAction) === '_grist_ACLRules' ||
+        getTableId(docAction) === '_grist_Resources')) {
       await this.update();
       return;
     }
@@ -656,7 +684,8 @@ export class GranularAccess {
   }
 
   // Compute which of the row ids supplied are for rows forbidden for this session.
-  private async _getForbiddenRows(docSession: OptDocSession, data: TableDataAction, ids: Set<number>): Promise<number[]> {
+  private async _getForbiddenRows(docSession: OptDocSession, data: TableDataAction, ids: Set<number>):
+      Promise<number[]> {
     const rec = new RecordView(data, undefined);
     const input: AclMatchInput = {user: await this._getUser(docSession), rec};
 
@@ -795,7 +824,10 @@ export class GranularAccess {
       try {
         // Use lodash's get() that supports paths, e.g. charId of 'a.b' would look up `user.a.b`.
         // TODO: add indexes to db.
-        rows = await this._fetchQueryFromDB({tableId: clause.tableId, filters: { [clause.lookupColId]: [get(user, clause.charId)] }});
+        rows = await this._fetchQueryFromDB({
+          tableId: clause.tableId,
+          filters: { [clause.lookupColId]: [get(user, clause.charId)] }
+        });
       } catch (e) {
         log.warn(`User attribute ${clause.name} failed`, e);
       }

app/server/lib/Sharing.ts

@@ -19,7 +19,7 @@ import {WorkCoordinator} from './WorkCoordinator';
 // processing hub actions or rebasing.
 interface UserRequest {
   action: UserActionBundle;
-  docSession: OptDocSession|null,
+  docSession: OptDocSession|null;
   resolve(result: UserResult): void;
   reject(err: Error): void;
 }

sandbox/grist/main.py

@@ -9,6 +9,7 @@ sys.path.append('thirdparty')
 import marshal
 import functools
 
+from acl_formula import parse_acl_formula
 import actions
 import sandbox
 import engine
@@ -110,6 +111,7 @@ def main():
   def get_formula_error(table_id, col_id, row_id):
     return objtypes.encode_object(eng.get_formula_error(table_id, col_id, row_id))
 
+  export(parse_acl_formula)
   export(eng.load_empty)
   export(eng.load_done)