(core) Improve the UI for ACL rules.

Summary:
- Add headers to tables.
- Change styles to reduce boxes-within-boxes.
- Add validation of table and column IDs, both in UI and on server when saving rules.
- Add autocomplete for tables/columns used for UserAttribute rules.
- Add a fancy widget to set permission bits.

Test Plan: Updated browser test for new UI, added a test case for user attributes.

Reviewers: paulfitz

Reviewed By: paulfitz

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

app/client/models/entities/ColumnRec.ts

@@ -81,8 +81,7 @@ export function createColumnRec(this: ColumnRec, docModel: DocModel): void {
   this.disableModify = ko.pureComputed(() => Boolean(this.summarySourceCol()));
   this.disableEditData = ko.pureComputed(() => Boolean(this.summarySourceCol()));
 
-  this.isHiddenCol = ko.pureComputed(() => this.colId().startsWith('gristHelper_') ||
+  this.isHiddenCol = ko.pureComputed(() => gristTypes.isHiddenCol(this.colId()));
-    this.colId() === 'manualSort');
 
   // Returns the rowModel for the referenced table, or null, if this is not a reference column.
   this.refTable = ko.pureComputed(() => {

app/client/ui/AccessRules.ts

@@ -5,20 +5,23 @@ import {GristDoc} from 'app/client/components/GristDoc';
 import {createObsArray} from 'app/client/lib/koArrayWrap';
 import {reportError, UserError} from 'app/client/models/errors';
 import {TableData} from 'app/client/models/TableData';
+import {PermissionKey, permissionsWidget} from 'app/client/ui/PermissionsWidget';
 import {shadowScroll} from 'app/client/ui/shadowScroll';
 import {bigBasicButton, bigPrimaryButton} from 'app/client/ui2018/buttons';
 import {colors, testId} from 'app/client/ui2018/cssVars';
-import {textInput} from 'app/client/ui2018/editableLabel';
+import {cssTextInput, textInput} from 'app/client/ui2018/editableLabel';
-import {icon} from 'app/client/ui2018/icons';
+import {cssIconButton, icon} from 'app/client/ui2018/icons';
-import {menu, menuItemAsync} from 'app/client/ui2018/menus';
+import {autocomplete, menu, menuItemAsync} from 'app/client/ui2018/menus';
-import {emptyPermissionSet, parsePermissions} from 'app/common/ACLPermissions';
+import {emptyPermissionSet} from 'app/common/ACLPermissions';
 import {PartialPermissionSet, permissionSetToText} 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';
+import {isHiddenCol} from 'app/common/gristTypes';
 import {isObject} from 'app/common/gutil';
 import {SchemaTypes} from 'app/common/schema';
-import {BaseObservable, Computed, Disposable, dom, MutableObsArray, obsArray, Observable, styled} from 'grainjs';
+import {BaseObservable, Computed, Disposable, MaybeObsArray, MutableObsArray, obsArray, Observable} from 'grainjs';
+import {dom, DomElementArg, styled} from 'grainjs';
 import isEqual = require('lodash/isEqual');
 
 // tslint:disable:max-classes-per-file no-console
@@ -85,6 +88,8 @@ export class AccessRules extends Disposable {
     this.update().catch(reportError);
   }
 
+  public get allTableIds() { return this._allTableIds; }
+
   /**
    * Replace internal state from the rules in DocData.
    */
@@ -198,7 +203,7 @@ export class AccessRules extends Disposable {
   }
 
   public buildDom() {
-    return [
+    return cssOuter(
       cssAddTableRow(
         bigBasicButton({disabled: true}, dom.hide(this._savingEnabled),
           dom.text((use) => {
@@ -217,7 +222,7 @@ export class AccessRules extends Disposable {
           testId('rules-revert'),
         ),
 
-        bigBasicButton('Add Table Rules', {style: 'margin-left: auto'},
+        bigBasicButton('Add Table Rules', cssDropdownIcon('Dropdown'), {style: 'margin-left: auto'},
           menu(() => [
             dom.forEach(this._allTableIds, (tableId) =>
               // Add the table on a timeout, to avoid disabling the clicked menu item
@@ -233,22 +238,44 @@ export class AccessRules extends Disposable {
       ),
       shadowScroll(
         dom.maybe(use => use(this._userAttrRules).length, () =>
-          cssTableRule(
+          cssSection(
-            cssTableHeader('User Attributes'),
+            cssSectionHeading('User Attributes'),
-            cssTableBody(
+            cssTableRounded(
-              dom.forEach(this._userAttrRules, (userAttr) => userAttr.buildDom()),
+              cssTableHeaderRow(
+                cssCell1(cssCell.cls('-rborder'), cssCell.cls('-center'), cssColHeaderCell('Name')),
+                cssCell4(
+                  cssColumnGroup(
+                    cssCell1(cssColHeaderCell('User Attribute')),
+                    cssCell1(cssColHeaderCell('Lookup Table')),
+                    cssCell1(cssColHeaderCell('Lookup Column')),
+                    cssCellIcon(),
+                  ),
+                ),
+              ),
+              dom.forEach(this._userAttrRules, (userAttr) => userAttr.buildUserAttrDom()),
             ),
           ),
         ),
         dom.forEach(this._tableRules, (tableRules) => tableRules.buildDom()),
-        cssTableRule(
+        cssSection(
-          cssTableHeader('Default Rules'),
+          cssSectionHeading('Default Rules'),
-          cssTableBody(
+          cssTableRounded(
-            dom.maybe(this._docDefaultRuleSet, ruleSet => ruleSet.buildDom()),
+            cssTableHeaderRow(
+              cssCell1(cssCell.cls('-rborder'), cssCell.cls('-center'), cssColHeaderCell('Columns')),
+              cssCell4(
+                cssColumnGroup(
+                  cssCellIcon(),
+                  cssCell2(cssColHeaderCell('Condition')),
+                  cssCell1(cssColHeaderCell('Permissions')),
+                  cssCellIcon(),
                 )
               )
             ),
-    ];
+            dom.maybe(this._docDefaultRuleSet, ruleSet => ruleSet.buildRuleSetDom()),
+          )
+        )
+      ),
+    );
   }
 
   /**
@@ -275,6 +302,28 @@ export class AccessRules extends Disposable {
     }
   }
 
+  // Check if the given tableId, and optionally a list of colIds, are present in this document.
+  // Returns '' if valid, or an error string if not. Exempt colIds will not trigger an error.
+  public checkTableColumns(tableId: string, colIds?: string[], exemptColIds?: string[]): string {
+    if (!tableId) { return ''; }
+    const table = this._gristDoc.docData.getTable(tableId);
+    if (!table) { return `Invalid table: ${tableId}`; }
+    if (colIds) {
+      const validColIds = new Set([...table.getColIds(), ...exemptColIds || []]);
+      const invalidColIds = colIds.filter(c => !validColIds.has(c));
+      if (invalidColIds.length === 0) { return ''; }
+      return `Invalid columns in table ${tableId}: ${invalidColIds.join(', ')}`;
+    }
+    return '';
+  }
+
+  // Returns a list of valid colIds for the given table, or undefined if the table isn't valid.
+  public getValidColIds(tableId: string): string[]|undefined {
+    return this._gristDoc.docData.getTable(tableId)?.getColIds()
+      .filter(id => !isHiddenCol(id))
+      .sort();
+  }
+
   private _addTableRules(tableId: string) {
     if (this._tableRules.get().some(t => t.tableId === tableId)) {
       throw new Error(`Trying to add TableRules for existing table ${tableId}`);
@@ -284,7 +333,7 @@ export class AccessRules extends Disposable {
   }
 
   private _addUserAttributes() {
-    this._userAttrRules.push(ObsUserAttributeRule.create(this._userAttrRules, this));
+    this._userAttrRules.push(ObsUserAttributeRule.create(this._userAttrRules, this, undefined, {focus: true}));
   }
 }
 
@@ -332,8 +381,8 @@ class TableRules extends Disposable {
   }
 
   public buildDom() {
-    return cssTableRule(
+    return cssSection(
-      cssTableHeader(
+      cssSectionHeading(
         dom('span', 'Rules for table ', cssTableName(this.tableId)),
         cssIconButton(icon('Dots'), {style: 'margin-left: auto'},
           menu(() => [
@@ -346,10 +395,22 @@ class TableRules extends Disposable {
         ),
         testId('rule-table-header'),
       ),
-      cssTableBody(
+      cssTableRounded(
-        dom.forEach(this._columnRuleSets, ruleSet => ruleSet.buildDom()),
+        cssTableHeaderRow(
-        dom.maybe(this._defaultRuleSet, ruleSet => ruleSet.buildDom()),
+          cssCell1(cssCell.cls('-rborder'), cssCell.cls('-center'), cssColHeaderCell('Columns')),
+          cssCell4(
+            cssColumnGroup(
+              cssCellIcon(),
+              cssCell2(cssColHeaderCell('Condition')),
+              cssCell1(cssColHeaderCell('Permissions')),
+              cssCellIcon(),
+            )
           ),
+        ),
+        dom.forEach(this._columnRuleSets, ruleSet => ruleSet.buildRuleSetDom()),
+        dom.maybe(this._defaultRuleSet, ruleSet => ruleSet.buildRuleSetDom()),
+      ),
+      dom.forEach(this._columnRuleSets, c => cssConditionError(dom.text(c.formulaError))),
       testId('rule-table'),
     );
   }
@@ -402,7 +463,9 @@ class TableRules extends Disposable {
   }
 
   private _addColumnRuleSet() {
-    this._columnRuleSets.push(ColumnObsRuleSet.create(this._columnRuleSets, this._accessRules, this, undefined, []));
+    this._columnRuleSets.push(ColumnObsRuleSet.create(this._columnRuleSets, this._accessRules, this, undefined, [],
+      {focus: true}
+    ));
   }
 
   private _addDefaultRuleSet() {
@@ -418,21 +481,18 @@ abstract class ObsRuleSet extends Disposable {
   // Whether rules changed, and if they are valid. Never unchanged if this._ruleSet is undefined.
   public ruleStatus: Computed<RuleStatus>;
 
-  // Whether the rule set includes any conditions besides the default rule.
-  public haveConditions: Computed<boolean>;
-
   // List of individual rule parts for this entity. The default permissions may be included as the
   // last rule part, with an empty aclFormula.
   private _body = this.autoDispose(obsArray<ObsRulePart>());
 
   // ruleSet is omitted for a new ObsRuleSet added by the user.
-  constructor(public accessRules: AccessRules, private _tableRules: TableRules|null, private _ruleSet?: RuleSet) {
+  constructor(public accessRules: AccessRules, protected _tableRules: TableRules|null, private _ruleSet?: RuleSet) {
     super();
     if (this._ruleSet) {
       this._body.set(this._ruleSet.body.map(part => ObsRulePart.create(this._body, this, part)));
     } else {
       // If creating a new RuleSet, start with just a default permission part.
-      this._body.set([ObsRulePart.create(this._body, this, undefined, true)]);
+      this._body.set([ObsRulePart.create(this._body, this, undefined)]);
     }
 
     this.ruleStatus = Computed.create(this, this._body, (use, body) => {
@@ -442,8 +502,6 @@ abstract class ObsRuleSet extends Disposable {
         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));
   }
 
   public getRules(tableId: string): RuleRec[] {
@@ -458,7 +516,20 @@ abstract class ObsRuleSet extends Disposable {
     return '*';
   }
 
-  public abstract buildDom(): Element;
+  public abstract buildResourceDom(): DomElementArg;
+
+  public buildRuleSetDom() {
+    return cssTableRow(
+      cssCell1(cssCell.cls('-rborder'),
+        this.buildResourceDom(),
+        testId('rule-resource')
+      ),
+      cssCell4(cssRuleBody.cls(''),
+        dom.forEach(this._body, part => part.buildRulePartDom()),
+      ),
+      testId('rule-set'),
+    );
+  }
 
   public removeRulePart(rulePart: ObsRulePart) {
     removeItem(this._body, rulePart);
@@ -469,7 +540,7 @@ abstract class ObsRuleSet extends Disposable {
 
   public addRulePart(beforeRule: ObsRulePart) {
     const i = this._body.get().indexOf(beforeRule);
-    this._body.splice(i, 0, ObsRulePart.create(this._body, this, undefined, false));
+    this._body.splice(i, 0, ObsRulePart.create(this._body, this, undefined));
   }
 
   /**
@@ -497,37 +568,53 @@ abstract class ObsRuleSet extends Disposable {
     return body[body.length - 1] === part;
   }
 
-  protected buildRuleBody() {
+  /**
-    return cssRuleSetBody(
+   * Which permission bits to allow the user to set.
-      dom.forEach(this._body, part => part.buildDom()),
+   */
-    );
+  public getAvailableBits(): PermissionKey[] {
+    if (this._tableRules) {
+      return ['read', 'update', 'create', 'delete'];
+    } else {
+      // For the doc-wide rule set, expose the schemaEdit bit too.
+      return ['read', 'update', 'create', 'delete', 'schemaEdit'];
+    }
   }
 }
 
 class ColumnObsRuleSet extends ObsRuleSet {
+  // Error message for this rule set, or '' if valid.
+  public formulaError: Computed<string>;
+
   private _colIds = Observable.create<string[]>(this, this._initialColIds);
   private _colIdStr = Computed.create(this, (use) => use(this._colIds).join(", "));
 
   constructor(accessRules: AccessRules, tableRules: TableRules, ruleSet: RuleSet|undefined,
-              private _initialColIds: string[]) {
+              private _initialColIds: string[], private _options: {focus?: boolean} = {}) {
     super(accessRules, tableRules, ruleSet);
+
+    this.formulaError = Computed.create(this, (use) => {
+      // Exempt existing colIds from checks, by including as a third argument.
+      return accessRules.checkTableColumns(tableRules.tableId, use(this._colIds), this._initialColIds);
+    });
+
     const baseRuleStatus = this.ruleStatus;
-    this.ruleStatus = Computed.create(this, (use) => Math.max(
+    this.ruleStatus = Computed.create(this, (use) => {
+      if (use(this.formulaError)) { return RuleStatus.Invalid; }
+      return Math.max(
         getChangedStatus(!isEqual(use(this._colIds), this._initialColIds)),
-        use(baseRuleStatus)
+        use(baseRuleStatus));
-    ));
+    });
   }
 
-  public buildDom() {
+  public buildResourceDom() {
     const saveColIds = async (colIdStr: string) => {
-      this._colIds.set(colIdStr.split(',').map(val => val.trim()).filter(Boolean));
+      this._colIds.set(colIdStr.split(/\W+/).map(val => val.trim()).filter(Boolean));
     };
-    return cssRuleSet(
+
-      cssResource('Columns', textInput(this._colIdStr, saveColIds),
+    return cssCellContent(
-        testId('rule-resource')
+      cssInput(this._colIdStr, saveColIds, {placeholder: 'Enter Columns'},
-      ),
+        (this._options.focus ? (elem) => { setTimeout(() => elem.focus(), 0); } : null),
-      this.buildRuleBody(),
+      )
-      testId('rule-set'),
     );
   }
 
@@ -538,6 +625,11 @@ class ColumnObsRuleSet extends ObsRuleSet {
   public getColIds(): string {
     return this._colIds.get().join(",");
   }
+
+  public getAvailableBits(): PermissionKey[] {
+    // Create/Delete bits can't be set on a column-specific rule.
+    return ['read', 'update'];
+  }
 }
 
 class DefaultObsRuleSet extends ObsRuleSet {
@@ -545,49 +637,91 @@ class DefaultObsRuleSet extends ObsRuleSet {
               private _haveColumnRules?: Observable<boolean>, ruleSet?: RuleSet) {
     super(accessRules, tableRules, ruleSet);
   }
-  public buildDom() {
+  public buildResourceDom() {
-    return cssRuleSet(
+    return [
-      cssResource(dom.text(use => this._haveColumnRules && use(this._haveColumnRules) ?
+      cssCenterContent.cls(''),
-        'Remaining Columns' : 'All Columns'),
+      dom.text(use => this._haveColumnRules && use(this._haveColumnRules) ? 'All Other' : 'All'),
-        testId('rule-resource')
+    ];
-      ),
-      this.buildRuleBody(),
-      testId('rule-set'),
-    );
   }
 }
 
 class ObsUserAttributeRule extends Disposable {
   public ruleStatus: Computed<RuleStatus>;
 
+  // If the rule failed validation, the error message to show. Blank if valid.
+  public formulaError: Computed<string>;
+
   private _name = Observable.create<string>(this, this._userAttr?.name || '');
   private _tableId = Observable.create<string>(this, this._userAttr?.tableId || '');
   private _lookupColId = Observable.create<string>(this, this._userAttr?.lookupColId || '');
   private _charId = Observable.create<string>(this, this._userAttr?.charId || '');
+  private _validColIds = Computed.create(this, this._tableId, (use, tableId) =>
+    this._accessRules.getValidColIds(tableId) || []);
 
-  constructor(private _accessRules: AccessRules, private _userAttr?: UserAttributeRule) {
+  constructor(private _accessRules: AccessRules, private _userAttr?: UserAttributeRule,
+              private _options: {focus?: boolean} = {}) {
     super();
-    this.ruleStatus = Computed.create(this, use =>
+    this.formulaError = Computed.create(this, this._tableId, this._lookupColId, (use, tableId, colId) => {
-      getChangedStatus(
+      // Don't check for errors if it's an existing rule and hasn't changed.
+      if (use(this._tableId) === this._userAttr?.tableId &&
+          use(this._lookupColId) === this._userAttr?.lookupColId) {
+        return '';
+      }
+      return _accessRules.checkTableColumns(tableId, colId ? [colId] : undefined);
+    });
+    this.ruleStatus = Computed.create(this, use => {
+      if (use(this.formulaError)) { return RuleStatus.Invalid; }
+      return getChangedStatus(
         use(this._name) !== this._userAttr?.name ||
         use(this._tableId) !== this._userAttr?.tableId ||
         use(this._lookupColId) !== this._userAttr?.lookupColId ||
         use(this._charId) !== this._userAttr?.charId
-      ));
+      );
+    });
+
+    // Reset lookupColId when tableId changes, since a colId from a different table would usually be wrong
+    this.autoDispose(this._tableId.addListener(() => this._lookupColId.set('')));
   }
 
-  public buildDom() {
+  public buildUserAttrDom() {
-    return cssUserAttribute(
+    return cssTableRow(
-      cssConditionInput(this._name, async (val) => this._name.set(val),
+      cssCell1(cssCell.cls('-rborder'),
-        {placeholder: 'New attribute name'}),
+        cssCellContent(
-      cssConditionInput(this._tableId, async (val) => this._tableId.set(val),
+          cssInput(this._name, async (val) => this._name.set(val),
-        {placeholder: 'Table ID'}),
+            {placeholder: 'Attribute name'},
-      cssConditionInput(this._lookupColId, async (val) => this._lookupColId.set(val),
+            (this._options.focus ? (elem) => { setTimeout(() => elem.focus(), 0); } : null),
-        {placeholder: 'Column to look up'}),
+            testId('rule-userattr-name'),
-      cssConditionInput(this._charId, async (val) => this._charId.set(val),
+          ),
-        {placeholder: 'User attribute to look up'}),
+        ),
-      cssIconButton(icon('Remove'), {style: 'margin-left: 4px'},
+      ),
+      cssCell4(cssRuleBody.cls(''),
+        cssColumnGroup(
+          cssCell1(
+            cssInput(this._charId, async (val) => this._charId.set(val),
+              {placeholder: 'Attribute to look up'},
+              testId('rule-userattr-attr'),
+            ),
+          ),
+          cssCell1(
+            inputAutocomplete(this._tableId, this._accessRules.allTableIds,
+              cssTextInput.cls(''), cssInput.cls(''), {placeholder: 'Table'},
+              testId('rule-userattr-table'),
+            ),
+          ),
+          cssCell1(
+            inputAutocomplete(this._lookupColId, this._validColIds,
+              cssTextInput.cls(''), cssInput.cls(''), {placeholder: 'Column'},
+              testId('rule-userattr-col'),
+            ),
+          ),
+          cssCellIcon(
+            cssIconButton(icon('Remove'),
               dom.on('click', () => this._accessRules.removeUserAttributes(this)))
+          ),
+          dom.maybe(this.formulaError, (msg) => cssConditionError(msg, testId('rule-error'))),
+        ),
+      ),
+      testId('rule-userattr'),
     );
   }
 
@@ -624,8 +758,6 @@ class ObsRulePart extends Disposable {
   private _permissions = Observable.create<PartialPermissionSet>(
     this, this._rulePart?.permissions || emptyPermissionSet());
 
-  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);
 
@@ -633,8 +765,7 @@ class ObsRulePart extends Disposable {
   private _formulaError = Observable.create(this, '');
 
   // rulePart is omitted for a new ObsRulePart added by the user.
-  constructor(private _ruleSet: ObsRuleSet, private _rulePart?: RulePart,
+  constructor(private _ruleSet: ObsRuleSet, private _rulePart?: RulePart) {
-              public readonly isDefault: boolean = (_rulePart?.aclFormula === '')) {
     super();
     this.ruleStatus = Computed.create(this, (use) => {
       if (use(this._formulaError)) { return RuleStatus.Invalid; }
@@ -657,17 +788,19 @@ class ObsRulePart extends Disposable {
     };
   }
 
-  public buildDom() {
+  public buildRulePartDom() {
-    return cssRulePart(
+    return cssColumnGroup(
+      cssCellIcon(
         (this._isNonFirstBuiltIn() ?
-        cssIconSpace({style: 'margin-right: 4px'}) :
+          null :
-        cssIconButton(icon('Plus'), {style: 'margin-right: 4px'},
+          cssIconButton(icon('Plus'),
             dom.on('click', () => this._ruleSet.addRulePart(this)),
             testId('rule-add'),
           )
         ),
-      cssCondition(
+      ),
-        cssConditionInput(
+      cssCell2(
+        cssInput(
           this._aclFormula, this._setAclFormula.bind(this),
           dom.prop('disabled', this.isBuiltIn()),
           dom.prop('placeholder', (use) => {
@@ -679,20 +812,23 @@ class ObsRulePart extends Disposable {
           }),
           testId('rule-acl-formula'),
         ),
-        dom.maybe(this._formulaError, (msg) => cssConditionError(msg, testId('rule-error'))),
       ),
-      cssPermissionsInput(
+      cssCell1(cssCell.cls('-stretch'),
-        this._permissionsText, async (p) => this._permissions.set(parsePermissions(p)),
+        permissionsWidget(this._ruleSet.getAvailableBits(), this._permissions,
-        dom.prop('disabled', this.isBuiltIn()),
+          {disabled: this.isBuiltIn()},
           testId('rule-permissions')
         ),
+      ),
+      cssCellIcon(
         (this.isBuiltIn() ?
-        cssIconSpace({style: 'margin-left: 4px'}) :
+          null :
-        cssIconButton(icon('Remove'), {style: 'margin-left: 4px'},
+          cssIconButton(icon('Remove'),
             dom.on('click', () => this._ruleSet.removeRulePart(this)),
             testId('rule-remove'),
           )
         ),
+      ),
+      dom.maybe(this._formulaError, (msg) => cssConditionError(msg, testId('rule-error'))),
       testId('rule-part'),
     );
   }
@@ -835,22 +971,48 @@ function getChangedStatus(value: boolean): RuleStatus {
   return value ? RuleStatus.ChangedValid : RuleStatus.Unchanged;
 }
 
+function inputAutocomplete(value: Observable<string>, choices: MaybeObsArray<string>, ...args: DomElementArg[]) {
+  function doSet() {
+    value.set(elem.value);
+  }
+  const elem = autocomplete(
+    dom('input', {type: 'text'},
+      dom.attr('value', value),
+      dom.on('change', doSet),
+      dom.on('blur', doSet),
+      ...args
+    ),
+    choices,
+    {onClick: doSet},
+  );
+  dom.onKeyElem(elem, 'keydown', {Enter: doSet});
+  return elem;
+}
+
+const cssOuter = styled('div', `
+  height: 100%;
+  width: 100%;
+  max-width: 800px;
+  margin: 0 auto;
+  display: flex;
+  flex-direction: column;
+`);
+
 const cssAddTableRow = styled('div', `
-  margin: 16px 64px 0 64px;
+  margin: 16px 16px 8px 16px;
   display: flex;
   gap: 16px;
 `);
 
-const cssTableRule = styled('div', `
+const cssDropdownIcon = styled(icon, `
-  margin: 24px 64px;
+  margin: -2px -2px 0 4px;
 `);
 
-const cssTableBody = styled('div', `
+const cssSection = styled('div', `
-  border: 2px solid ${colors.slate};
+  margin: 16px 16px 24px 16px;
-  border-radius: 8px;
 `);
 
-const cssTableHeader = styled('div', `
+const cssSectionHeading = styled('div', `
   display: flex;
   align-items: center;
   margin-bottom: 8px;
@@ -862,84 +1024,111 @@ const cssTableName = styled('span', `
   color: ${colors.dark};
 `);
 
-const cssRuleSet = styled('div', `
+const cssInput = styled(textInput, `
+  width: 100%;
+  border: 1px solid transparent;
+  cursor: pointer;
+
+  &:hover {
+    border: 1px solid ${colors.darkGrey};
+  }
+  &:focus {
+    box-shadow: inset 0 0 0 1px var(--grist-color-cursor);
+    border: 1px solid var(--grist-color-cursor);
+    cursor: unset;
+  }
+  &[disabled] {
+    color: ${colors.dark};
+    background-color: ${colors.mediumGreyOpaque};
+    box-shadow: unset;
+    border: unset;
+  }
+`);
+
+const cssConditionError = styled('div', `
+  margin-top: 4px;
+  width: 100%;
+  color: ${colors.error};
+`);
+
+/**
+ * Fairly general table styles.
+ */
+const cssTableRounded = styled('div', `
+  border: 1px solid ${colors.slate};
+  border-radius: 8px;
+  overflow: hidden;
+`);
+
+// Row with a border
+const cssTableRow = styled('div', `
   display: flex;
-  border-bottom: 2px solid ${colors.slate};
+  border-bottom: 1px solid ${colors.slate};
   &:last-child {
     border-bottom: none;
   }
 `);
 
-const cssResource = styled('div', `
+// Darker table header
-  flex: 1;
+const cssTableHeaderRow = styled(cssTableRow, `
-  display: flex;
+  background-color: ${colors.mediumGrey};
-  flex-direction: column;
+  color: ${colors.dark};
-  border-right: 2px solid ${colors.slate};
-  padding: 8px;
-  min-width: 0;
 `);
 
-
+// Cell for table column header.
-const cssRuleSetBody = styled('div', `
+const cssColHeaderCell = styled('div', `
-  flex: 4;
+  margin: 4px 8px;
-  display: flex;
+  text-transform: uppercase;
-  flex-direction: column;
+  font-weight: 500;
-  padding: 8px;
+  font-size: 10px;
-  min-width: 0;
 `);
 
-const cssRulePart = styled('div', `
+// General table cell.
+const cssCell = styled('div', `
+  min-width: 0px;
+  overflow: hidden;
+
+  &-rborder {
+    border-right: 1px solid ${colors.slate};
+  }
+  &-center {
+    text-align: center;
+  }
+  &-stretch {
+    min-width: unset;
+    overflow: visible;
+  }
+`);
+
+// Variations on columns of different widths.
+const cssCellIcon = styled(cssCell, `flex: none; width: 24px;`);
+const cssCell1 = styled(cssCell, `flex: 1;`);
+const cssCell2 = styled(cssCell, `flex: 2;`);
+const cssCell4 = styled(cssCell, `flex: 4;`);
+
+// Group of columns, which may be placed inside a cell.
+const cssColumnGroup = styled('div', `
   display: flex;
-  align-items: start;
+  align-items: center;
+  gap: 0px 8px;
+  margin: 0 8px;
+  flex-wrap: wrap;
+`);
+
+const cssRuleBody = styled('div', `
+  display: flex;
+  flex-direction: column;
+  justify-content: center;
+  gap: 4px;
   margin: 4px 0;
 `);
 
-const cssCondition = styled('div', `
+const cssCellContent = styled('div', `
-  min-width: 0;
+  margin: 4px 8px;
-  flex: 1;
-  display: flex;
-  flex-direction: column;
 `);
 
-const cssConditionInput = styled(textInput, `
+const cssCenterContent = styled('div', `
-  &[disabled] {
-    background-color: ${colors.mediumGreyOpaque};
-    color: ${colors.dark};
-  }
-`);
-
-const cssConditionError = styled('div', `
-  color: ${colors.error};
-`);
-
-const cssPermissionsInput = styled(cssConditionInput, `
-  margin-left: 8px;
-  width: 64px;
-  flex: none;
-`);
-
-const cssIconSpace = styled('div', `
-  flex: none;
-  height: 24px;
-  width: 24px;
-  margin: 2px;
-`);
-
-const cssIconButton = styled(cssIconSpace, `
-  padding: 4px;
-  border-radius: 3px;
-  line-height: 0px;
-  cursor: default;
-  --icon-color: ${colors.slate};
-  &:hover {
-    background-color: ${colors.darkGrey};
-    --icon-color: ${colors.slate};
-  }
-`);
-
-const cssUserAttribute = styled('div', `
   display: flex;
   align-items: center;
-  gap: 16px;
+  justify-content: center;
-  margin: 16px 8px;
 `);

app/client/ui/PermissionsWidget.ts

@@ -0,0 +1,175 @@
+/**
+ * Implements a widget showing 3-state boxes for permissions
+ * (for Allow / Deny / Pass-Through).
+ */
+import {colors, testId} from 'app/client/ui2018/cssVars';
+import {cssIconButton, icon} from 'app/client/ui2018/icons';
+import {menu, menuIcon, menuItem} from 'app/client/ui2018/menus';
+import {PartialPermissionSet, PartialPermissionValue} from 'app/common/ACLPermissions';
+import {ALL_PERMISSION_PROPS, emptyPermissionSet} from 'app/common/ACLPermissions';
+import {capitalize} from 'app/common/gutil';
+import {dom, DomElementArg, Observable, styled} from 'grainjs';
+import isEqual = require('lodash/isEqual');
+
+// One of the strings 'read', 'update', etc.
+export type PermissionKey = keyof PartialPermissionSet;
+
+/**
+ * Renders a box for each of availableBits, and a dropdown with a description and some shortcuts.
+ */
+export function permissionsWidget(
+  availableBits: PermissionKey[],
+  pset: Observable<PartialPermissionSet>,
+  options: {disabled: boolean},
+  ...args: DomElementArg[]
+) {
+  // These are the permission sets available to set via the dropdown.
+  const empty: PartialPermissionSet = emptyPermissionSet();
+  const allowAll: PartialPermissionSet = makePermissionSet(availableBits, () => 'allow');
+  const denyAll: PartialPermissionSet = makePermissionSet(availableBits, () => 'deny');
+  const readOnly: PartialPermissionSet = makePermissionSet(availableBits, (b) => b === 'read' ? 'allow' : 'deny');
+
+  return cssPermissions(
+    dom.forEach(availableBits, (bit) => {
+      return cssBit(
+        bit.slice(0, 1).toUpperCase(),              // Show the first letter of the property (e.g. "R" for "read")
+        cssBit.cls((use) => '-' + use(pset)[bit]),  // -allow, -deny class suffixes.
+        dom.attr('title', (use) => capitalize(`${use(pset)[bit]} ${bit}`.trim())),    // Explanation on hover
+        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])}))
+        )
+      );
+    }),
+    cssIconButton(icon('Dropdown'), testId('permissions-dropdown'), menu(() => {
+      // Show a disabled "Custom" menu item if the permission set isn't a recognized one, for
+      // information purposes.
+      const isCustom = [allowAll, denyAll, readOnly, empty].every(ps => !isEqual(ps, pset.get()));
+      return [
+        (isCustom ?
+          cssMenuItem(() => null, dom.cls('disabled'), menuIcon('Tick'),
+            cssMenuItemContent(
+              'Custom',
+              cssMenuItemDetails(dom.text((use) => psetDescription(use(pset))))
+            ),
+          ) :
+          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',
+          dom.cls('disabled', options.disabled)
+        ),
+        cssMenuItem(() => pset.set(denyAll), tick(isEqual(pset.get(), denyAll)), 'Deny All',
+          dom.cls('disabled', options.disabled)
+        ),
+        cssMenuItem(() => pset.set(readOnly), tick(isEqual(pset.get(), readOnly)), 'Read Only',
+          dom.cls('disabled', options.disabled)
+        ),
+        cssMenuItem(() => pset.set(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'],
+          dom.cls('disabled', options.disabled),
+        ),
+      ];
+    })),
+    ...args
+  );
+}
+
+function next(pvalue: PartialPermissionValue): PartialPermissionValue {
+  switch (pvalue) {
+    case 'allow': return '';
+    case 'deny': return 'allow';
+  }
+  return 'deny';
+}
+
+// Helper to build up permission sets.
+function makePermissionSet(bits: PermissionKey[], makeValue: (bit: PermissionKey) => PartialPermissionValue) {
+  const pset = emptyPermissionSet();
+  for (const bit of bits) {
+    pset[bit] = makeValue(bit);
+  }
+  return pset;
+}
+
+// Helper for a tick (checkmark) icon, replacing it with an equialent space when not shown.
+function tick(show: boolean) {
+  return show ? menuIcon('Tick') : cssMenuIconSpace();
+}
+
+// Human-readable summary of the permission set. E.g. "Allow Read. Deny Update, Create.".
+function psetDescription(permissionSet: PartialPermissionSet): string {
+  const allow: string[] = [];
+  const deny: string[] = [];
+  for (const prop of ALL_PERMISSION_PROPS) {
+    const value = permissionSet[prop];
+    if (value === "allow") {
+      allow.push(capitalize(prop));
+    } else if (value === "deny") {
+      deny.push(capitalize(prop));
+    }
+  }
+  const parts: string[] = [];
+  if (allow.length) { parts.push(`Allow ${allow.join(", ")}.`); }
+  if (deny.length) { parts.push(`Deny ${deny.join(", ")}.`); }
+  return parts.join(' ');
+}
+
+const cssPermissions = styled('div', `
+  display: flex;
+  gap: 4px;
+`);
+
+const cssBit = styled('div', `
+  flex: none;
+  height: 24px;
+  width: 24px;
+  border-radius: 2px;
+  font-size: 13px;
+  font-weight: 500;
+  border: 1px dashed ${colors.darkGrey};
+  color: ${colors.darkGrey};
+  cursor: pointer;
+
+  display: flex;
+  align-items: center;
+  justify-content: center;
+
+  &-allow {
+    background-color: ${colors.lightGreen};
+    border: 1px solid ${colors.lightGreen};
+    color: white;
+  }
+  &-deny {
+    background-image: linear-gradient(-45deg, ${colors.error} 14px, white 15px 16px, ${colors.error} 16px);
+    border: 1px solid ${colors.error};
+    color: white;
+  }
+  &.disabled {
+    opacity: 0.5;
+  }
+`);
+
+const cssMenuIconSpace = styled('div', `
+  width: 24px;
+`);
+
+// Don't make disabled item too hard to see here.
+const cssMenuItem = styled(menuItem, `
+  align-items: start;
+  &.disabled {
+    opacity: unset;
+  }
+`);
+
+const cssMenuItemContent = styled('div', `
+  display: flex;
+  flex-direction: column;
+`);
+
+const cssMenuItemDetails = styled('div', `
+  font-size: 12px;
+`);

app/client/ui2018/icons.ts

@@ -49,6 +49,7 @@
  *   `);
  */
 
+import { colors } from 'app/client/ui2018/cssVars';
 import { dom, DomElementArg, styled } from 'grainjs';
 import { IconName } from './IconList';
 
@@ -73,3 +74,21 @@ export function icon(name: IconName, ...domArgs: DomElementArg[]): HTMLElement {
     ...domArgs
   );
 }
+
+/**
+ * Container box for an slate-colored icon to serve as a button, with a grey background on hover.
+ */
+export const cssIconButton = styled('div', `
+  flex: none;
+  height: 24px;
+  width: 24px;
+  padding: 4px;
+  border-radius: 3px;
+  line-height: 0px;
+  cursor: default;
+  --icon-color: ${colors.slate};
+  &:hover {
+    background-color: ${colors.darkGrey};
+    --icon-color: ${colors.slate};
+  }
+`);

app/client/ui2018/menus.ts

@@ -209,7 +209,7 @@ export function autocomplete(
 ) {
   return weasel.autocomplete(inputElem, choices, {
     ...defaults, ...options,
-    menuCssClass: menuCssClass + ' ' + cssSelectMenuElem.className,
+    menuCssClass: defaults.menuCssClass + ' ' + cssSelectMenuElem.className + ' ' + (options.menuCssClass || '')
   });
 }
 

app/common/ACLRuleCollection.ts

@@ -1,6 +1,6 @@
 import { parsePermissions } from 'app/common/ACLPermissions';
 import { ILogger } from 'app/common/BaseAPI';
-import { RowRecord } from 'app/common/DocActions';
+import { CellValue, RowRecord } from 'app/common/DocActions';
 import { DocData } from 'app/common/DocData';
 import { AclMatchFunc, ParsedAclFormula, RulePart, RuleSet, UserAttributeRule } from 'app/common/GranularAccessClause';
 import { getSetMapValue } from 'app/common/gutil';
@@ -20,7 +20,7 @@ const DEFAULT_RULE_SET: RuleSet = {
   }, {
     aclFormula: "user.Access in ['viewers']",
     matchFunc:  (input) => ['viewers'].includes(String(input.user.Access)),
-    permissions: parsePermissions('+R'),
+    permissions: parsePermissions('+R-CUDS'),
     permissionsText: '+R',
   }, {
     aclFormula: "",
@@ -166,6 +166,55 @@ export class ACLRuleCollection {
     this._userAttributeRules = userAttributeMap;
   }
 
+  /**
+   * Check that all references to table and column IDs in ACL rules are valid.
+   */
+  public checkDocEntities(docData: DocData) {
+    const tablesTable = docData.getTable('_grist_Tables')!;
+    const columnsTable = docData.getTable('_grist_Tables_column')!;
+
+    // Collect valid tableIds and check rules against those.
+    const validTableIds = new Set(tablesTable.getColValues('tableId'));
+    const invalidTables = this.getAllTableIds().filter(t => !validTableIds.has(t));
+    if (invalidTables.length > 0) {
+      throw new Error(`Invalid tables in rules: ${invalidTables.join(', ')}`);
+    }
+
+    // Collect valid columns, grouped by tableRef (rowId of table record).
+    const validColumns = new Map<number, Set<CellValue>>();   // Map from tableRef to set of colIds.
+    const colTableRefs = columnsTable.getColValues('parentId')!;
+    for (const [i, colId] of columnsTable.getColValues('colId')!.entries()) {
+      getSetMapValue(validColumns, colTableRefs[i], () => new Set()).add(colId);
+    }
+
+    // For each table, check that any explicitly mentioned columns are valid.
+    for (const tableId of this.getAllTableIds()) {
+      const tableRef = tablesTable.findRow('tableId', tableId);
+      const validTableCols = validColumns.get(tableRef);
+      for (const ruleSet of this.getAllColumnRuleSets(tableId)) {
+        if (Array.isArray(ruleSet.colIds)) {
+          const invalidColIds = ruleSet.colIds.filter(c => !validTableCols?.has(c));
+          if (invalidColIds.length > 0) {
+            throw new Error(`Invalid columns in rules for table ${tableId}: ${invalidColIds.join(', ')}`);
+          }
+        }
+      }
+    }
+
+    // Check for valid tableId/lookupColId combinations in UserAttribute rules.
+    const invalidUAColumns: string[] = [];
+    for (const rule of this.getUserAttributeRules().values()) {
+      const tableRef = tablesTable.findRow('tableId', rule.tableId);
+      const colRef = columnsTable.findMatchingRowId({parentId: tableRef, colId: rule.lookupColId});
+      if (!colRef) {
+        invalidUAColumns.push(`${rule.tableId}.${rule.lookupColId}`);
+      }
+    }
+    if (invalidUAColumns.length > 0) {
+      throw new Error(`Invalid columns in User Attribute rules: ${invalidUAColumns.join(', ')}`);
+    }
+  }
+
   private _safeReadAclRules(docData: DocData, options: ReadAclOptions): ReadAclResults {
     try {
       this.ruleError = undefined;

app/common/gristTypes.ts

@@ -28,6 +28,11 @@ export const enum GristObjCode {
 
 export const MANUALSORT = 'manualSort';
 
+// Whether a column is internal and should be hidden.
+export function isHiddenCol(colId: string): boolean {
+  return colId.startsWith('gristHelper_') || colId === MANUALSORT;
+}
+
 // This mapping includes both the default value, and its representation for SQLite.
 const _defaultValues: {[key in GristType]: [CellValue, string]} = {
   'Any':              [ null,  "NULL"  ],

app/server/lib/GranularAccess.ts

@@ -156,12 +156,19 @@ export class GranularAccess {
         docActions.map((action, idx) => this._checkIncomingDocAction(docSession, action, idx)));
     }
 
+    if (this._recoveryMode) {
+      // Don't do any further checking in recovery mode.
+      return;
+    }
+
     // 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)))) {
+    if (docActions.some(docAction => ['_grist_ACLRules', '_grist_ACLResources'].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_Tables: this._docData.getTable('_grist_Tables')!.getTableDataAction(),
+          _grist_Tables_column: this._docData.getTable('_grist_Tables_column')!.getTableDataAction(),
           _grist_ACLResources: this._docData.getTable('_grist_ACLResources')!.getTableDataAction(),
           _grist_ACLRules: this._docData.getTable('_grist_ACLRules')!.getTableDataAction(),
         });
@@ -172,9 +179,14 @@ export class GranularAccess {
       // 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) {
+      if (ruleCollection.ruleError) {
         throw new ApiError(ruleCollection.ruleError.message, 400);
       }
+      try {
+        ruleCollection.checkDocEntities(tmpDocData);
+      } catch (err) {
+        throw new ApiError(err.message, 400);
+      }
     }
   }