(core) Update ACL resources/rules when tables/columns get renamed

Summary:
- Placed rule-updating functions in acl.py.
- Reset UI when rules update externally, or alert the user to reset if there
  are pending local changes.
- Removed some unused and distracting bits from client-side DocModel.

A few improvements related to poor error handling:
- In case of missing DocActions (tickled by broken ACL rule handling), don't
  add to confusion by attempting to process bad actions
- In case of missing attributes in ACL formulas, return undefined rather than
  fail; the latter creates more problems.
- In case in invalid rules, fail rather than skip; this feels more correct now
  that we have error checking and recovery option, and helps avoid invalid rules.
- Prevent saving invalid rules with an empty ACL formula.
- Fix bug with rule positions.

Test Plan: Added a python and browser test for table/column renames.

Reviewers: paulfitz

Reviewed By: paulfitz

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

app/client/aclui/AccessRules.ts

@@ -79,6 +79,9 @@ export class AccessRules extends Disposable {
   // Whether the save button should be enabled.
   private _savingEnabled: Computed<boolean>;
 
+  // Error or warning message to show next to Save/Reset buttons if non-empty.
+  private _errorMessage = Observable.create(this, '');
+
   constructor(private _gristDoc: GristDoc) {
     super();
     this._ruleStatus = Computed.create(this, (use) => {
@@ -117,7 +120,25 @@ export class AccessRules extends Disposable {
       return result;
     });
 
-    this.update().catch(reportError);
+    // The UI in this module isn't really dynamic (that would be tricky while allowing unsaved
+    // changes). Instead, react deliberately if rules change. Note that table/column renames would
+    // trigger changes to rules, so we don't need to listen for those separately.
+    for (const tableId of ['_grist_ACLResources', '_grist_ACLRules']) {
+      const tableData = this._gristDoc.docData.getTable(tableId)!;
+      this.autoDispose(tableData.tableActionEmitter.addListener(this._onChange, this));
+    }
+    this.update().catch((e) => this._errorMessage.set(e.message));
+  }
+
+  public _onChange() {
+    if (this._ruleStatus.get() === RuleStatus.Unchanged) {
+      // If no changes, it's safe to just reload the rules from docData.
+      this.update().catch((e) => this._errorMessage.set(e.message));
+    } else {
+      this._errorMessage.set(
+        'Access rules have changed. Click Reset to revert your changes and refresh the rules.'
+      );
+    }
   }
 
   public get allTableIds() { return this._allTableIds; }
@@ -128,6 +149,7 @@ export class AccessRules extends Disposable {
    * Replace internal state from the rules in DocData.
    */
   public async update() {
+    this._errorMessage.set('');
     const rules = this._ruleCollection;
     await rules.update(this._gristDoc.docData, {log: console});
     this._tableRules.set(
@@ -209,7 +231,7 @@ export class AccessRules extends Disposable {
       if (pos && pos > lastGoodRulePos) {
         const step = (pos - lastGoodRulePos) / (i - lastGoodIndex);
         for (let k = lastGoodIndex + 1; k < i; k++) {
-          newRules[k].rulePos = step * (k - lastGoodIndex);
+          newRules[k].rulePos = lastGoodRulePos + step * (k - lastGoodIndex);
         }
         lastGoodRulePos = pos;
         lastGoodIndex = i;
@@ -251,7 +273,7 @@ export class AccessRules extends Disposable {
           dom.on('click', () => this.save()),
           testId('rules-save'),
         ),
-        bigBasicButton('Revert', dom.show(this._savingEnabled),
+        bigBasicButton('Reset', dom.show(use => use(this._ruleStatus) !== RuleStatus.Unchanged),
           dom.on('click', () => this.update()),
           testId('rules-revert'),
         ),
@@ -270,6 +292,9 @@ export class AccessRules extends Disposable {
         ),
         bigBasicButton('Add User Attributes', dom.on('click', () => this._addUserAttributes())),
       ),
+      cssConditionError(dom.text(this._errorMessage), {style: 'margin-left: 16px'},
+        testId('access-rules-error')
+      ),
       shadowScroll(
         dom.maybe(use => use(this._userAttrRules).length, () =>
           cssSection(
@@ -541,7 +566,9 @@ abstract class ObsRuleSet extends Disposable {
     return this._body.get().map(part => ({
       ...part.getRulePart(),
       resourceRec: {tableId, colIds: this.getColIds()}
-    }));
+    }))
+    // Skip entirely empty rule parts: they are invalid and dropping them is the best fix.
+    .filter(part => part.aclFormula || part.permissionsText);
   }
 
   public getColIds(): string {
@@ -723,8 +750,9 @@ class ObsUserAttributeRule extends Disposable {
       const result = use(this._accessRules.userAttrChoices).filter(c => (c.ruleIndex < index));
 
       // If the currently-selected option isn't one of the choices, insert it too.
-      if (!result.some(choice => (choice.value === this._charId.get()))) {
-        result.unshift({ruleIndex: -1, value: this._charId.get(), label: `user.${this._charId.get()}`});
+      const charId = use(this._charId);
+      if (charId && !result.some(choice => (choice.value === charId))) {
+        result.unshift({ruleIndex: -1, value: charId, label: `user.${charId}`});
       }
       return result;
     });
@@ -817,11 +845,23 @@ class ObsRulePart extends Disposable {
   // If the formula failed validation, the error message to show. Blank if valid.
   private _formulaError = Observable.create(this, '');
 
+  // Error message if any validation failed.
+  private _error: Computed<string>;
+
   // rulePart is omitted for a new ObsRulePart added by the user.
   constructor(private _ruleSet: ObsRuleSet, private _rulePart?: RulePart) {
     super();
+    this._error = Computed.create(this, (use) => {
+      return use(this._formulaError) ||
+        ( !this._ruleSet.isLastCondition(use, this) &&
+          use(this._aclFormula) === '' &&
+          permissionSetToText(use(this._permissions)) !== '' ?
+          'Condition cannot be blank' : ''
+        );
+    });
+
     this.ruleStatus = Computed.create(this, (use) => {
-      if (use(this._formulaError)) { return RuleStatus.Invalid; }
+      if (use(this._error)) { return RuleStatus.Invalid; }
       if (use(this._checkPending)) { return RuleStatus.CheckPending; }
       return getChangedStatus(
         use(this._aclFormula) !== this._rulePart?.aclFormula ||
@@ -883,7 +923,7 @@ class ObsRulePart extends Disposable {
           )
         ),
       ),
-      dom.maybe(this._formulaError, (msg) => cssConditionError(msg, testId('rule-error'))),
+      dom.maybe(this._error, (msg) => cssConditionError(msg, testId('rule-error'))),
       testId('rule-part'),
     );
   }

app/client/models/BaseRowModel.js

@@ -119,10 +119,12 @@ BaseRowModel.prototype._process_RenameColumn = function(action, tableId, oldColI
   // handle standard renames differently
   if (this._fields.indexOf(newColId) !== -1) {
     console.error("RowModel #RenameColumn %s %s %s: already exists", tableId, oldColId, newColId);
+    return;
   }
   var index = this._fields.indexOf(oldColId);
   if (index === -1) {
     console.error("RowModel #RenameColumn %s %s %s: not found", tableId, oldColId, newColId);
+    return;
   }
   this._fields[index] = newColId;
 

app/client/models/DocModel.ts

@@ -24,9 +24,6 @@ import * as rowset from 'app/client/models/rowset';
 import {RowId} from 'app/client/models/rowset';
 import {schema, SchemaTypes} from 'app/common/schema';
 
-import {ACLMembershipRec, createACLMembershipRec} from 'app/client/models/entities/ACLMembershipRec';
-import {ACLPrincipalRec, createACLPrincipalRec} from 'app/client/models/entities/ACLPrincipalRec';
-import {ACLResourceRec, createACLResourceRec} from 'app/client/models/entities/ACLResourceRec';
 import {ColumnRec, createColumnRec} from 'app/client/models/entities/ColumnRec';
 import {createDocInfoRec, DocInfoRec} from 'app/client/models/entities/DocInfoRec';
 import {createPageRec, PageRec} from 'app/client/models/entities/PageRec';
@@ -41,8 +38,6 @@ import {createViewSectionRec, ViewSectionRec} from 'app/client/models/entities/V
 
 // Re-export all the entity types available. The recommended usage is like this:
 //    import {ColumnRec, ViewFieldRec} from 'app/client/models/DocModel';
-export {ACLMembershipRec} from 'app/client/models/entities/ACLMembershipRec';
-export {ACLPrincipalRec} from 'app/client/models/entities/ACLPrincipalRec';
 export {ColumnRec} from 'app/client/models/entities/ColumnRec';
 export {DocInfoRec} from 'app/client/models/entities/DocInfoRec';
 export {PageRec} from 'app/client/models/entities/PageRec';
@@ -115,9 +110,6 @@ export class DocModel {
   public tabBar: MTM<TabBarRec> = this._metaTableModel("_grist_TabBar", createTabBarRec);
   public validations: MTM<ValidationRec> = this._metaTableModel("_grist_Validations", createValidationRec);
   public replHist: MTM<REPLRec> = this._metaTableModel("_grist_REPL_Hist", createREPLRec);
-  public aclPrincipals: MTM<ACLPrincipalRec> = this._metaTableModel("_grist_ACLPrincipals", createACLPrincipalRec);
-  public aclMemberships: MTM<ACLMembershipRec> = this._metaTableModel("_grist_ACLMemberships", createACLMembershipRec);
-  public aclResources: MTM<ACLResourceRec> = this._metaTableModel("_grist_ACLResources", createACLResourceRec);
   public pages: MTM<PageRec> = this._metaTableModel("_grist_Pages", createPageRec);
 
   public allTables: KoArray<TableRec>;

app/client/models/entities/ACLMembershipRec.ts

@@ -1,14 +0,0 @@
-import {ACLPrincipalRec, DocModel, IRowModel, refRecord} from 'app/client/models/DocModel';
-import * as ko from 'knockout';
-
-// Table for containment relationships between Principals, e.g. user contains multiple
-// instances, group contains multiple users, and groups may contain other groups.
-export interface ACLMembershipRec extends IRowModel<"_grist_ACLMemberships"> {
-  parentRec: ko.Computed<ACLPrincipalRec>;
-  childRec: ko.Computed<ACLPrincipalRec>;
-}
-
-export function createACLMembershipRec(this: ACLMembershipRec, docModel: DocModel): void {
-  this.parentRec = refRecord(docModel.aclPrincipals, this.parent);
-  this.childRec = refRecord(docModel.aclPrincipals, this.child);
-}

app/client/models/entities/ACLPrincipalRec.ts

@@ -1,29 +0,0 @@
-import {KoArray} from 'app/client/lib/koArray';
-import {ACLMembershipRec, DocModel, IRowModel, recordSet} from 'app/client/models/DocModel';
-import {KoSaveableObservable} from 'app/client/models/modelUtil';
-import * as ko from 'knockout';
-
-// A principals used by ACL rules, including users, groups, and instances.
-export interface ACLPrincipalRec extends IRowModel<"_grist_ACLPrincipals"> {
-  // Declare a more specific type for 'type' than what's set automatically from schema.ts.
-  type: KoSaveableObservable<'user'|'instance'|'group'>;
-
-  // KoArray of ACLMembership row models which contain this principal as a child.
-  parentMemberships: ko.Computed<KoArray<ACLMembershipRec>>;
-
-  // Gives an array of ACLPrincipal parents to this row model.
-  parents: ko.Computed<ACLPrincipalRec[]>;
-
-  // KoArray of ACLMembership row models which contain this principal as a parent.
-  childMemberships: ko.Computed<KoArray<ACLMembershipRec>>;
-
-  // Gives an array of ACLPrincipal children of this row model.
-  children: ko.Computed<ACLPrincipalRec[]>;
-}
-
-export function createACLPrincipalRec(this: ACLPrincipalRec, docModel: DocModel): void {
-  this.parentMemberships = recordSet(this, docModel.aclMemberships, 'child');
-  this.childMemberships = recordSet(this, docModel.aclMemberships, 'parent');
-  this.parents = ko.pureComputed(() => this.parentMemberships().all().map(m => m.parentRec()));
-  this.children = ko.pureComputed(() => this.childMemberships().all().map(m => m.childRec()));
-}

app/client/models/entities/ACLResourceRec.ts

@@ -1,7 +0,0 @@
-import {DocModel, IRowModel} from 'app/client/models/DocModel';
-
-export type ACLResourceRec = IRowModel<"_grist_ACLResources">;
-
-export function createACLResourceRec(this: ACLResourceRec, docModel: DocModel): void {
-  // no extra fields
-}