(core) allow multiple rule sets for overlapping columns if they are all allows or all denies

Summary:
Previously, it was forbidden to have two rule sets with overlapping columns,
since that could introduce an dependency on order of evaluation without
the user having a way to control that order.  This diff permits such rule sets
if the are compatible in a very simple way -- all allows or all denies.
Anything more complicated (even if actually order independent) remains forbidden.

Test Plan: added tests

Reviewers: dsagal

Reviewed By: dsagal

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

app/client/aclui/AccessRules.ts

@@ -15,8 +15,8 @@ import {colors, testId} from 'app/client/ui2018/cssVars';
 import {textInput} from 'app/client/ui2018/editableLabel';
 import {cssIconButton, icon} from 'app/client/ui2018/icons';
 import {IOptionFull, menu, menuItemAsync} from 'app/client/ui2018/menus';
-import {emptyPermissionSet} from 'app/common/ACLPermissions';
+import {emptyPermissionSet, MixedPermissionValue} from 'app/common/ACLPermissions';
-import {PartialPermissionSet, permissionSetToText} from 'app/common/ACLPermissions';
+import {PartialPermissionSet, permissionSetToText, summarizePermissions, summarizePermissionSet} from 'app/common/ACLPermissions';
 import {ACLRuleCollection} from 'app/common/ACLRuleCollection';
 import {BulkColValues, RowRecord, UserAction} from 'app/common/DocActions';
 import {RulePart, RuleSet, UserAttributeRule} from 'app/common/GranularAccessClause';
@@ -490,17 +490,40 @@ class TableRules extends Disposable {
    */
   public getResources(): ResourceRec[] {
     // Check that the colIds are valid.
-    const seen = new Set<string>();
+    const seen = {
+      allow: new Set<string>(),   // columns mentioned in rules that only have 'allow's.
+      deny: new Set<string>(),    // columns mentioned in rules that only have 'deny's.
+      mixed: new Set<string>()    // columns mentioned in any rules.
+    };
     for (const ruleSet of this._columnRuleSets.get()) {
+      const sign = ruleSet.summarizePermissions();
+      const counterSign = sign === 'mixed' ? 'mixed' : (sign === 'allow' ? 'deny' : 'allow');
       const colIds = ruleSet.getColIdList();
       if (colIds.length === 0) {
         throw new UserError(`No columns listed in a column rule for table ${this.tableId}`);
       }
       for (const colId of colIds) {
-        if (seen.has(colId)) {
+        if (seen[counterSign].has(colId)) {
-          throw new UserError(`Column ${colId} appears in multiple rules for table ${this.tableId}`);
+          // There may be an order dependency between rules.  We've done a little analysis, to
+          // allow the useful pattern of forbidding all access to columns, and then adding back
+          // access to different sets for different teams/conditions (or allowing all access
+          // by default, and then forbidding different sets).  But if there's a mix of
+          // allows and denies, then we throw up our hands.
+          // TODO: could analyze more deeply.  An easy step would be to analyze per permission bit.
+          // Could also allow order dependency and provide a way to control the order.
+          // TODO: could be worth also flagging multiple rulesets with the same columns as
+          // undesirable.
+          throw new UserError(`Column ${colId} appears in multiple rules for table ${this.tableId}` +
+                              ` that might be order-dependent. Try splitting rules up differently?`);
+        }
+        if (sign === 'mixed') {
+          seen.allow.add(colId);
+          seen.deny.add(colId);
+          seen.mixed.add(colId);
+        } else {
+          seen[sign].add(colId);
+          seen.mixed.add(colId);
         }
-        seen.add(colId);
       }
     }
 
@@ -585,6 +608,15 @@ abstract class ObsRuleSet extends Disposable {
     return '*';
   }
 
+  /**
+   * Check if RuleSet may only add permissions, only remove permissions, or may do either.
+   * A rule that neither adds nor removes permissions is treated as mixed for simplicity,
+   * though this would be suboptimal if this were a useful case to support.
+   */
+  public summarizePermissions(): MixedPermissionValue {
+    return summarizePermissions(this._body.get().map(p => p.summarizePermissions()));
+  }
+
   public abstract buildResourceDom(): DomElementArg;
 
   public buildRuleSetDom() {
@@ -891,6 +923,15 @@ class ObsRulePart extends Disposable {
     };
   }
 
+  /**
+   * Check if RulePart may only add permissions, only remove permissions, or may do either.
+   * A rule that neither adds nor removes permissions is treated as mixed for simplicity,
+   * though this would be suboptimal if this were a useful case to support.
+   */
+  public summarizePermissions(): MixedPermissionValue {
+    return summarizePermissionSet(this._permissions.get());
+  }
+
   public buildRulePartDom() {
     return cssColumnGroup(
       cssCellIcon(

app/common/ACLPermissions.ts

@@ -161,3 +161,32 @@ export function mergePermissions<T, U>(psets: Array<PermissionSet<T>>, combine:
 export function toMixed(pset: PartialPermissionSet): MixedPermissionSet {
   return mergePermissions([pset], ([bit]) => (bit === 'allow' || bit === 'mixed' ? bit : 'deny'));
 }
+
+/**
+ * Check if PermissionSet may only add permissions, only remove permissions, or may do either.
+ * A rule that neither adds nor removes permissions is treated as mixed.
+ */
+export function summarizePermissionSet(pset: PartialPermissionSet): MixedPermissionValue {
+  let sign = '';
+  for (const key of Object.keys(pset) as Array<keyof PartialPermissionSet>) {
+    const pWithSome = pset[key];
+    // "Some" postfix is not significant for summarization.
+    const p = pWithSome === 'allowSome' ? 'allow' : (pWithSome === 'denySome' ? 'deny' : pWithSome)
+    if (!p || p === sign) { continue; }
+    if (!sign) {
+      sign = p;
+      continue;
+    }
+    sign = 'mixed';
+  }
+  return (sign === 'allow' || sign === 'deny') ? sign : 'mixed';
+}
+
+/**
+ * Summarize whether a set of permissions are all 'allow', all 'deny', or other ('mixed').
+ */
+export function summarizePermissions(perms: MixedPermissionValue[]): MixedPermissionValue {
+  if (perms.length === 0) { return 'mixed'; }
+  const perm = perms[0];
+  return perms.some(p => p !== perm) ? 'mixed' : perm;
+}