(core) alert user if they try to use rec in a column rule controlling read permission

Summary:
This particular combination of features is not built out - data will be
censored but changes to data will not.  So the user will now get an error
if they try to do it.  Existing rules of this kind will continue to
operate as before, and can be set via the api.

Test Plan: added test

Reviewers: dsagal

Reviewed By: dsagal

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

app/client/aclui/AccessRules.ts

@@ -18,8 +18,9 @@ import {IOptionFull, menu, menuItemAsync} from 'app/client/ui2018/menus';
 import {emptyPermissionSet, MixedPermissionValue} from 'app/common/ACLPermissions';
 import {PartialPermissionSet, permissionSetToText, summarizePermissions, summarizePermissionSet} from 'app/common/ACLPermissions';
 import {ACLRuleCollection} from 'app/common/ACLRuleCollection';
+import { ApiError } from 'app/common/ApiError';
 import {BulkColValues, RowRecord, UserAction} from 'app/common/DocActions';
-import {RulePart, RuleSet, UserAttributeRule} from 'app/common/GranularAccessClause';
+import {FormulaProperties, getFormulaProperties, RulePart, RuleSet, UserAttributeRule} from 'app/common/GranularAccessClause';
 import {isHiddenCol} from 'app/common/gristTypes';
 import {isObject} from 'app/common/gutil';
 import {SchemaTypes} from 'app/common/schema';
@@ -367,10 +368,11 @@ export class AccessRules extends Disposable {
     removeItem(this._userAttrRules, userAttr);
   }
 
-  public async checkAclFormula(text: string): Promise<void> {
+  public async checkAclFormula(text: string): Promise<FormulaProperties> {
     if (text) {
       return this._gristDoc.docComm.checkAclFormula(text);
     }
+    return {};
   }
 
   // Check if the given tableId, and optionally a list of colIds, are present in this document.
@@ -688,6 +690,13 @@ abstract class ObsRuleSet extends Disposable {
     const tableId = this._tableRules?.tableId;
     return (tableId && this.accessRules.getValidColIds(tableId)) || [];
   }
+
+  /**
+   * Check if this rule set is limited to a set of columns.
+   */
+  public hasColumns() {
+    return false;
+  }
 }
 
 class ColumnObsRuleSet extends ObsRuleSet {
@@ -730,6 +739,10 @@ class ColumnObsRuleSet extends ObsRuleSet {
     // Create/Delete bits can't be set on a column-specific rule.
     return ['read', 'update'];
   }
+
+  public hasColumns() {
+    return true;
+  }
 }
 
 class DefaultObsRuleSet extends ObsRuleSet {
@@ -887,6 +900,8 @@ class ObsRulePart extends Disposable {
   // If the formula failed validation, the error message to show. Blank if valid.
   private _formulaError = Observable.create(this, '');
 
+  private _formulaProperties = Observable.create<FormulaProperties>(this, getAclFormulaProperties(this._rulePart));
+
   // Error message if any validation failed.
   private _error: Computed<string>;
 
@@ -932,6 +947,25 @@ class ObsRulePart extends Disposable {
     return summarizePermissionSet(this._permissions.get());
   }
 
+  /**
+   * Verify that the rule is in a good state, optionally given a proposed permission change.
+   */
+  public sanityCheck(pset?: PartialPermissionSet) {
+    if (this._ruleSet.hasColumns() &&
+        (pset || this._permissions.get()).read &&
+        this._formulaProperties.get().hasRecOrNewRec) {
+      if (pset && pset.read && this._permissions.get().read) {
+        // We don't want users to set either allow or deny read permissions in
+        // row-dependent rules, but if such a permission is set, allow it to be
+        // toggled to enable the deny->allow->unset cycling in the UI.
+        return;
+      }
+      throw new ApiError('Control of the read permission for column rules is unavailable ' +
+                         'when the formula uses the rec variable. ' +
+                         'Sorry! We will get to it, promise.', 400);
+    }
+  }
+
   public buildRulePartDom() {
     return cssColumnGroup(
       cssCellIcon(
@@ -961,7 +995,7 @@ class ObsRulePart extends Disposable {
       ),
       cssCell1(cssCell.cls('-stretch'),
         permissionsWidget(this._ruleSet.getAvailableBits(), this._permissions,
-          {disabled: this.isBuiltIn()},
+          {disabled: this.isBuiltIn(), sanityCheck: (pset) => this.sanityCheck(pset)},
           testId('rule-permissions')
         ),
       ),
@@ -993,7 +1027,8 @@ class ObsRulePart extends Disposable {
     this._checkPending.set(true);
     this._formulaError.set('');
     try {
-      await this._ruleSet.accessRules.checkAclFormula(text);
+      this._formulaProperties.set(await this._ruleSet.accessRules.checkAclFormula(text));
+      this.sanityCheck();
     } catch (e) {
       this._formulaError.set(e.message);
     } finally {
@@ -1117,6 +1152,11 @@ function getChangedStatus(value: boolean): RuleStatus {
   return value ? RuleStatus.ChangedValid : RuleStatus.Unchanged;
 }
 
+function getAclFormulaProperties(part?: RulePart): FormulaProperties {
+  const aclFormulaParsed = part?.origRecord?.aclFormulaParsed;
+  return aclFormulaParsed ? getFormulaProperties(JSON.parse(String(aclFormulaParsed))) : {};
+}
+
 const cssOuter = styled('div', `
   flex: auto;
   height: 100%;

app/client/aclui/PermissionsWidget.ts

@@ -20,7 +20,7 @@ export type PermissionKey = keyof PartialPermissionSet;
 export function permissionsWidget(
   availableBits: PermissionKey[],
   pset: Observable<PartialPermissionSet>,
-  options: {disabled: boolean},
+  options: {disabled: boolean, sanityCheck?: (p: PartialPermissionSet) => void},
   ...args: DomElementArg[]
 ) {
   // These are the permission sets available to set via the dropdown.
@@ -28,6 +28,10 @@ export function permissionsWidget(
   const allowAll: PartialPermissionSet = makePermissionSet(availableBits, () => 'allow');
   const denyAll: PartialPermissionSet = makePermissionSet(availableBits, () => 'deny');
   const readOnly: PartialPermissionSet = makePermissionSet(availableBits, (b) => b === 'read' ? 'allow' : 'deny');
+  const setPermissions = (p: PartialPermissionSet) => {
+    options.sanityCheck?.(p);
+    pset.set(p);
+  };
 
   return cssPermissions(
     dom.forEach(availableBits, (bit) => {
@@ -38,7 +42,7 @@ export function permissionsWidget(
         dom.cls('disabled', options.disabled),
         // Cycle the bit's value on click, unless disabled.
         (options.disabled ? null :
-          dom.on('click', () => pset.set({...pset.get(), [bit]: next(pset.get()[bit])}))
+          dom.on('click', () => setPermissions({...pset.get(), [bit]: next(pset.get()[bit])}))
         )
       );
     }),
@@ -57,16 +61,16 @@ export function permissionsWidget(
           null
         ),
         // If the set matches any recognized pattern, mark that item with a tick (checkmark).
-        cssMenuItem(() => pset.set(allowAll), tick(isEqual(pset.get(), allowAll)), 'Allow All',
+        cssMenuItem(() => setPermissions(allowAll), tick(isEqual(pset.get(), allowAll)), 'Allow All',
           dom.cls('disabled', options.disabled)
         ),
-        cssMenuItem(() => pset.set(denyAll), tick(isEqual(pset.get(), denyAll)), 'Deny All',
+        cssMenuItem(() => setPermissions(denyAll), tick(isEqual(pset.get(), denyAll)), 'Deny All',
           dom.cls('disabled', options.disabled)
         ),
-        cssMenuItem(() => pset.set(readOnly), tick(isEqual(pset.get(), readOnly)), 'Read Only',
+        cssMenuItem(() => setPermissions(readOnly), tick(isEqual(pset.get(), readOnly)), 'Read Only',
           dom.cls('disabled', options.disabled)
         ),
-        cssMenuItem(() => pset.set(empty),
+        cssMenuItem(() => setPermissions(empty),
           // For the empty permission set, it seems clearer to describe it as "No Effect", but to
           // all it "Clear" when offering to the user as the action.
           isEqual(pset.get(), empty) ? [tick(true), 'No Effect'] : [tick(false), 'Clear'],

app/common/ActiveDocAPI.ts

@@ -1,5 +1,6 @@
 import {ActionGroup} from 'app/common/ActionGroup';
 import {CellValue, TableDataAction, UserAction} from 'app/common/DocActions';
+import {FormulaProperties} from 'app/common/GranularAccessClause';
 import {Peer} from 'app/common/sharing';
 import {UploadResult} from 'app/common/uploads';
 import {ParseOptions} from 'app/plugin/FileParserAPI';
@@ -226,7 +227,7 @@ export interface ActiveDocAPI {
   /**
    * Check if an ACL formula is valid. If not, will throw an error with an explanation.
    */
-  checkAclFormula(text: string): Promise<void>;
+  checkAclFormula(text: string): Promise<FormulaProperties>;
 
   /**
    * Returns the full set of tableIds, with the list of colIds for each table. This is intended

app/common/GranularAccessClause.ts

@@ -57,6 +57,13 @@ export type AclMatchFunc = (input: AclMatchInput) => boolean;
  */
 export type ParsedAclFormula = [string, ...Array<ParsedAclFormula|CellValue>];
 
+/**
+ * Observations about a formula.
+ */
+export interface FormulaProperties {
+  hasRecOrNewRec?: boolean;
+}
+
 export interface UserAttributeRule {
   origRecord?: RowRecord;         // Original record used to create this UserAttributeRule.
   name: string;       // Should be unique among UserAttributeRules.
@@ -64,3 +71,26 @@ export interface UserAttributeRule {
   lookupColId: string;  // Column in tableId in which to do the lookup.
   charId: string;     // Attribute to look up, possibly a path. E.g. 'Email' or 'office.city'.
 }
+
+/**
+ * Check some key facts about the formula.
+ */
+export function getFormulaProperties(formula: ParsedAclFormula) {
+  const result: FormulaProperties = {}
+  if (usesRec(formula)) { result.hasRecOrNewRec = true; }
+  return result;
+}
+
+/**
+ * Check whether a formula mentions `rec` or `newRec`.
+ */
+export function usesRec(formula: ParsedAclFormula): boolean {
+  if (!Array.isArray(formula)) { throw new Error('expected a list'); }
+  if (formula[0] === 'Name' && (formula[1] === 'rec' || formula[1] === 'newRec')) {
+    return true;
+  }
+  return formula.some(el => {
+    if (!Array.isArray(el)) { return false; }
+    return usesRec(el as any);
+  });
+}

app/server/lib/ActiveDoc.ts

@@ -30,6 +30,7 @@ import {toTableDataAction} from 'app/common/DocActions';
 import {DocData} from 'app/common/DocData';
 import {DocSnapshots} from 'app/common/DocSnapshot';
 import {EncActionBundleFromHub} from 'app/common/EncActionBundle';
+import {FormulaProperties, getFormulaProperties} from 'app/common/GranularAccessClause';
 import {byteString, countIf} from 'app/common/gutil';
 import {InactivityTimer} from 'app/common/InactivityTimer';
 import * as marshal from 'app/common/marshal';
@@ -933,15 +934,16 @@ export class ActiveDoc extends EventEmitter {
   /**
    * 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> {
+  public async checkAclFormula(docSession: DocSession, text: string): Promise<FormulaProperties> {
     // Checks can leak names of tables and columns.
-    if (await this._granularAccess.hasNuancedAccess(docSession)) { return; }
+    if (await this._granularAccess.hasNuancedAccess(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.
+      return getFormulaProperties(parsedAclFormula);
     } catch (e) {
       e.message = e.message?.replace('[Sandbox] ', '');
       throw e;