(core) granular access control in the presence of schema changes

Summary:
 - Support schema changes in the presence of non-trivial ACL rules.
 - Fix update of `aclFormulaParsed` when updating formulas automatically after schema change.
 - Filter private metadata in broadcasts, not just fetches.  Censorship method is unchanged, just refactored.
 - Allow only owners to change ACL rules.
 - Force reloads if rules are changed.
 - Track rule changes within bundle, for clarity during schema changes - tableId and colId changes create a muddle otherwise.
 - Show or forbid pages dynamically depending on user's access to its sections. Logic unchanged, just no longer requires reload.
 - Fix calculation of pre-existing rows touched by a bundle, in the presence of schema changes.
 - Gray out acl page for non-owners.

Test Plan: added tests

Reviewers: dsagal

Reviewed By: dsagal

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

app/client/aclui/ACLUsers.ts

@@ -60,6 +60,7 @@ export class ACLUsersPopup extends Disposable {
     const doc = pageModel.currentDoc.get();
     if (doc) {
       const permissionData = await pageModel.appModel.api.getDocAccess(doc.id);
+      if (this.isDisposed()) { return; }
       this._usersInDoc = permissionData.users.map(user => ({
         ...user,
         access: getRealAccess(user, permissionData),

app/client/aclui/AccessRules.ts

@@ -152,6 +152,7 @@ export class AccessRules extends Disposable {
    * Replace internal state from the rules in DocData.
    */
   public async update() {
+    if (this.isDisposed()) { return; }
     this._errorMessage.set('');
     const rules = this._ruleCollection;
     [ , , this._aclResources] = await Promise.all([

app/client/components/Comm.ts

@@ -447,7 +447,7 @@ export class Comm extends dispose.Disposable implements GristServerAPI, DocListA
       const reqId = message.reqId;
       const r = this.pendingRequests.get(reqId);
       if (r) {
-        this.pendingRequests.delete(reqId);
+        try {
           if ('errorCode' in message && message.errorCode === 'AUTH_NO_VIEW') {
             // We should only arrive here if the user had view access, and then lost it.
             // We should not let the user see the document any more.  Let's reload the
@@ -474,6 +474,9 @@ export class Comm extends dispose.Disposable implements GristServerAPI, DocListA
             console.log(`Comm response #${reqId} ${r.methodName} OK`);
             r.resolve(message.data);
           }
+        } finally {
+          this.pendingRequests.delete(reqId);
+        }
       } else {
         console.log("Comm: Response to unknown reqId " + reqId);
       }
@@ -514,8 +517,8 @@ export class Comm extends dispose.Disposable implements GristServerAPI, DocListA
     }
     if (error) {
       console.log("Comm: Rejecting req #" + reqId + " " + r.methodName + ": " + error);
-      this.pendingRequests.delete(reqId);
       r.reject(new Error('Comm: ' + error));
+      this.pendingRequests.delete(reqId);
     }
   }
 

app/client/components/DocComm.ts

@@ -17,6 +17,7 @@ export interface DocUserAction extends CommMessage {
   data: {
     docActions: DocAction[];
     actionGroup: ActionGroup;
+    error?: string;
   };
 }
 
@@ -76,7 +77,15 @@ export class DocComm extends Disposable implements ActiveDocAPI {
     this.listenTo(_comm, 'docShutdown', (m: CommMessage) => {
       if (this.isActionFromThisDoc(m)) { this._isClosed = true; }
     });
-    this.onDispose(() => this._shutdown());
+    this.onDispose(async () => {
+      try {
+        await this._shutdown();
+      } catch (e) {
+        if (!String(e).match(/GristWSConnection disposed/)) {
+          reportError(e);
+        }
+      }
+    });
   }
 
   // Returns the URL params that identifying this open document to the DocWorker

app/client/components/GristDoc.ts

@@ -293,6 +293,18 @@ export class GristDoc extends DisposableWithEvents {
   public onDocUserAction(message: DocUserAction) {
     console.log("GristDoc.onDocUserAction", message);
     let schemaUpdated = false;
+    /**
+     * If an operation is applied successfully to a document, and then information about
+     * it is broadcast to clients, and one of those broadcasts has a failure (due to
+     * granular access control, which is client-specific), then that error is logged on
+     * the server and also sent to the client via an `error` field.  Under normal operation,
+     * there should be no such errors, but if they do arise it is best to make them as visible
+     * as possible.
+     */
+    if (message.data.error) {
+      reportError(new Error(message.data.error));
+      return;
+    }
     if (this.docComm.isActionFromThisDoc(message)) {
       const docActions = message.data.docActions;
       for (let i = 0, len = docActions.length; i < len; i++) {

app/client/models/DocPageModel.ts

@@ -294,18 +294,18 @@ function addMenu(importSources: ImportSource[], gristDoc: GristDoc, isReadonly:
   const selectBy = gristDoc.selectBy.bind(gristDoc);
   return [
     menuItem(
-      (elem) => openPageWidgetPicker(elem, gristDoc.docModel, (val) => gristDoc.addNewPage(val),
+      (elem) => openPageWidgetPicker(elem, gristDoc.docModel, (val) => gristDoc.addNewPage(val).catch(reportError),
                                      {isNewPage: true, buttonLabel: 'Add Page'}),
       menuIcon("Page"), "Add Page", testId('dp-add-new-page'),
       dom.cls('disabled', isReadonly)
     ),
     menuItem(
-      (elem) => openPageWidgetPicker(elem, gristDoc.docModel, (val) => gristDoc.addWidgetToPage(val),
+      (elem) => openPageWidgetPicker(elem, gristDoc.docModel, (val) => gristDoc.addWidgetToPage(val).catch(reportError),
                                      {isNewPage: false, selectBy}),
       menuIcon("Widget"), "Add Widget to Page", testId('dp-add-widget-to-page'),
       dom.cls('disabled', isReadonly)
     ),
-    menuItem(() => gristDoc.addEmptyTable(), menuIcon("TypeTable"), "Add Empty Table", testId('dp-empty-table'),
+    menuItem(() => gristDoc.addEmptyTable().catch(reportError), menuIcon("TypeTable"), "Add Empty Table", testId('dp-empty-table'),
             dom.cls('disabled', isReadonly)),
     menuDivider(),
     ...importSources.map((importSource, i) =>

app/client/models/errors.ts

@@ -48,6 +48,10 @@ export function getAppErrors(): string[] {
  */
 export function reportError(err: Error|string): void {
   log.error(`ERROR:`, err);
+  if (String(err).match(/GristWSConnection disposed/)) {
+    // This error can be emitted while a page is reloaded, and isn't worth reporting.
+    return;
+  }
   _logError(err);
   if (_notifier && !_notifier.isDisposed()) {
     if (!isError(err)) {

app/client/ui/LeftPanelCommon.ts

@@ -103,6 +103,11 @@ export const cssPageEntry = styled('div', `
     color: ${colors.light};
     --icon-color: ${colors.light};
   }
+  &-disabled, &-disabled:hover, &-disabled.weasel-popup-open {
+    background-color: initial;
+    color: ${colors.mediumGrey};
+    --icon-color: ${colors.mediumGrey};
+  }
   .${cssTools.className}-collapsed > & {
     margin-right: 0;
   }
@@ -126,6 +131,19 @@ export const cssPageLink = styled('a', `
   }
 `);
 
+// Styled like a cssPageLink, but in a disabled mode, without an actual link.
+export const cssPageDisabledLink = styled('span', `
+  display: flex;
+  align-items: center;
+  height: 32px;
+  line-height: 32px;
+  padding-left: 24px;
+  outline: none;
+  text-decoration: none;
+  outline: none;
+  color: inherit;
+`);
+
 export const cssLinkText = styled('span', `
   white-space: nowrap;
   overflow: hidden;

app/client/ui/Tools.ts

@@ -3,7 +3,7 @@ import { urlState } from "app/client/models/gristUrlState";
 import { showExampleCard } from 'app/client/ui/ExampleCard';
 import { examples } from 'app/client/ui/ExampleInfo';
 import { createHelpTools, cssSectionHeader, cssSpacer, cssTools } from 'app/client/ui/LeftPanelCommon';
-import { cssLinkText, cssPageEntry, cssPageIcon, cssPageLink } from 'app/client/ui/LeftPanelCommon';
+import { cssLinkText, cssPageDisabledLink, cssPageEntry, cssPageIcon, cssPageLink } from 'app/client/ui/LeftPanelCommon';
 import { colors } from 'app/client/ui2018/cssVars';
 import { icon } from 'app/client/ui2018/icons';
 import { Disposable, dom, makeTestId, Observable, styled } from "grainjs";
@@ -12,7 +12,7 @@ const testId = makeTestId('test-tools-');
 
 export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Observable<boolean>): Element {
   const aclUIEnabled = Boolean(urlState().state.get().params?.aclUI);
-
+  const isOwner = gristDoc.docPageModel.currentDoc.get()?.access === 'owners';
   return cssTools(
     cssTools.cls('-collapsed', (use) => !use(leftPanelOpen)),
     cssSectionHeader("TOOLS"),
@@ -20,9 +20,10 @@ export function tools(owner: Disposable, gristDoc: GristDoc, leftPanelOpen: Obse
     (aclUIEnabled ?
       cssPageEntry(
         cssPageEntry.cls('-selected', (use) => use(gristDoc.activeViewId) === 'acl'),
-        cssPageLink(cssPageIcon('EyeShow'),
+        cssPageEntry.cls('-disabled', !isOwner),
+        (isOwner ? cssPageLink : cssPageDisabledLink)(cssPageIcon('EyeShow'),
           cssLinkText('Access Rules'),
-          urlState().setLinkUrl({docPage: 'acl'})
+          isOwner ? urlState().setLinkUrl({docPage: 'acl'}) : null
         ),
         testId('access-rules'),
       ) :

app/client/ui2018/pages.ts

@@ -28,11 +28,6 @@ type NameType = Observable<string>|ko.Observable<string>;
 // the item in the menu.
 export function buildPageDom(name: NameType, actions: PageActions, ...args: DomElementArg[]) {
 
-  // If name is blank, this page is censored, so don't include any options for manipulation.
-  // We can get fancier about this later.
-  const initName = ('peek' in name) ? name.peek() : name.get();
-  if (initName === '') { return dom('div', '-'); }
-
   const isRenaming = observable(false);
   const pageMenu = () => [
     menuItem(() => isRenaming.set(true), "Rename", testId('rename'),
@@ -57,6 +52,7 @@ export function buildPageDom(name: NameType, actions: PageActions, ...args: DomE
   return pageElem = dom(
     'div',
     dom.autoDispose(lis),
+    domComputed((use) => use(name) === '', blank => blank ? dom('div', '-') :
       domComputed(isRenaming, (isrenaming) => (
         isrenaming ?
           cssPageItem(
@@ -93,7 +89,7 @@ export function buildPageDom(name: NameType, actions: PageActions, ...args: DomE
             args
           )
       )),
-  );
+    ));
 }
 
 const cssPageItem = styled('a', `

app/common/GranularAccessClause.ts

@@ -22,12 +22,19 @@ export interface RulePart {
   memo?: string;
 }
 
-// Light wrapper around characteristics or records.
+// Light wrapper for reading records or user attributes.
 export interface InfoView {
   get(key: string): CellValue;
   toJSON(): {[key: string]: any};
 }
 
+// As InfoView, but also supporting writing.
+export interface InfoEditor {
+  get(key: string): CellValue;
+  set(key: string, val: CellValue): this;
+  toJSON(): {[key: string]: any};
+}
+
 // Represents user info, which may include properties which are themselves RowRecords.
 export type UserInfo = Record<string, CellValue|InfoView|Record<string, string>>;
 

app/server/lib/ActiveDoc.ts

@@ -57,7 +57,7 @@ import {DocSession, getDocSessionAccess, getDocSessionUser, getDocSessionUserId,
         makeExceptionalDocSession, OptDocSession} from './DocSession';
 import {DocStorage} from './DocStorage';
 import {expandQuery} from './ExpandedQuery';
-import {GranularAccess} from './GranularAccess';
+import {GranularAccess, GranularAccessForBundle} from './GranularAccess';
 import {OnDemandActions} from './OnDemandActions';
 import {findOrAddAllEnvelope, Sharing} from './Sharing';
 
@@ -419,7 +419,7 @@ export class ActiveDoc extends EventEmitter {
     this._onDemandActions = new OnDemandActions(this.docStorage, this.docData);
 
     await this._actionHistory.initialize();
-    this._granularAccess = new GranularAccess(this.docData, (query) => {
+    this._granularAccess = new GranularAccess(this.docData, this.docClients, (query) => {
       return this._fetchQueryFromDB(query, false);
     }, this.recoveryMode, this._docManager.getHomeDbManager(), this.docName);
     await this._granularAccess.update();
@@ -1064,43 +1064,14 @@ export class ActiveDoc extends EventEmitter {
   /**
    * Called by Sharing manager when working on modifying the document.
    * Called when DocActions have been produced from UserActions, but
-   * before those DocActions have been applied to the DB, to confirm
+   * before those DocActions have been applied to the DB. GranularAccessBundle
-   * that those DocActions are legal according to any granular access
+   * methods can confirm that those DocActions are legal according to any
-   * rules.
+   * granular access rules.
    */
-  public async canApplyDocActions(docSession: OptDocSession, docActions: DocAction[], undo: DocAction[]) {
+  public getGranularAccessForBundle(docSession: OptDocSession, docActions: DocAction[], undo: DocAction[],
-    return this._granularAccess.canApplyDocActions(docSession, docActions, undo);
+                                    userActions: UserAction[]): GranularAccessForBundle {
-  }
+    this._granularAccess.getGranularAccessForBundle(docSession, docActions, undo, userActions);
-
+    return this._granularAccess;
-  /**
-   * Called by Sharing manager when working on modifying the document.
-   * Called when DocActions have been produced from UserActions, and
-   * have been applied to the DB, but before the changes have been
-   * broadcast to clients.
-   */
-  public async appliedActions(docActions: DocAction[], undo: DocAction[]) {
-    await this._granularAccess.appliedActions(docActions, undo);
-  }
-
-  /**
-   * Called by Sharing manager when done working on modifying the document,
-   * regardless of whether the modification succeeded or failed.
-   */
-  public async finishedActions() {
-    await this._granularAccess.finishedActions();
-  }
-
-  /**
-   * Broadcast document changes to all the document's clients.  Doesn't involve
-   * ActiveDoc directly, but placed here to facilitate future work on granular
-   * access control.
-   */
-  public async broadcastDocUpdate(client: Client|null, type: string, message: {
-    actionGroup: ActionGroup,
-    docActions: DocAction[]
-  }) {
-    await this.docClients.broadcastDocMessage(client, 'docUserAction', message,
-                                              (docSession) => this._filterDocUpdate(docSession, message));
   }
 
   /**
@@ -1385,23 +1356,6 @@ export class ActiveDoc extends EventEmitter {
     log.origLog(level, `ActiveDoc ` + msg, ...args, this.getLogMeta(docSession));
   }
 
-  /**
-   * This filters a message being broadcast to all clients to be appropriate for one
-   * particular client, if that client may need some material filtered out.
-   */
-  private async _filterDocUpdate(docSession: OptDocSession, message: {
-    actionGroup: ActionGroup,
-    docActions: DocAction[]
-  }) {
-    if (await this._granularAccess.canReadEverything(docSession)) { return message; }
-    const result = {
-      actionGroup: await this._granularAccess.filterActionGroup(docSession, message.actionGroup),
-      docActions: await this._granularAccess.filterOutgoingDocActions(docSession, message.docActions),
-    };
-    if (result.docActions.length === 0) { return null; }
-    return result;
-  }
-
   /**
    * Called before a migration.  Makes sure a back-up is made.
    */

app/server/lib/DocClients.ts

@@ -97,7 +97,7 @@ export class DocClients {
             if (e.code && e.code === 'NEED_RELOAD') {
               sendDocMessage(curr.client, curr.fd, 'docShutdown', null, fromSelf);
             } else {
-              throw e;
+              sendDocMessage(curr.client, curr.fd, 'docUserAction', {error: String(e)}, fromSelf);
             }
           }
         }

app/server/lib/PermissionInfo.ts

@@ -14,7 +14,7 @@ import { mapValues } from 'lodash';
  */
 export interface PermissionSetWithContextOf<T = PermissionSet> {
   perms: T;
-  ruleType: 'full'|'table'|'column';
+  ruleType: 'full'|'table'|'column'|'row';
   getMemos: () => MemoSet;
 }
 
@@ -111,11 +111,18 @@ export class MemoInfo extends RuleInfo<MemoSet, MemoSet> {
   }
 }
 
+export interface IPermissionInfo {
+  getColumnAccess(tableId: string, colId: string): MixedPermissionSetWithContext;
+  getTableAccess(tableId: string): TablePermissionSetWithContext;
+  getFullAccess(): MixedPermissionSetWithContext;
+  getRuleCollection(): ACLRuleCollection;
+}
+
 /**
  * Helper for evaluating rules given a particular user and optionally a record. It evaluates rules
  * for a column, table, or document, with caching to avoid evaluating the same rule multiple times.
  */
-export class PermissionInfo extends RuleInfo<MixedPermissionSet, TablePermissionSet> {
+export class PermissionInfo extends RuleInfo<MixedPermissionSet, TablePermissionSet> implements IPermissionInfo {
   private _ruleResults = new Map<RuleSet, MixedPermissionSet>();
 
   // Get permissions for "tableId:colId", defaulting to "tableId:*" and "*:*" as needed.
@@ -138,7 +145,7 @@ export class PermissionInfo extends RuleInfo<MixedPermissionSet, TablePermission
   public getTableAccess(tableId: string): TablePermissionSetWithContext {
     return {
       perms: this.getTableAspect(tableId),
-      ruleType: 'table',
+      ruleType: this._input?.rec ? 'row' : 'table',
       getMemos: () => new MemoInfo(this._acls, this._input).getTableAspect(tableId)
     };
   }
@@ -154,6 +161,10 @@ export class PermissionInfo extends RuleInfo<MixedPermissionSet, TablePermission
     };
   }
 
+  public getRuleCollection() {
+    return this._acls;
+  }
+
   protected _processRule(ruleSet: RuleSet, defaultAccess?: () => MixedPermissionSet): MixedPermissionSet {
     return getSetMapValue(this._ruleResults, ruleSet, () => {
       const pset = evaluateRule(ruleSet, this._input);

app/server/lib/RowAccess.ts

@@ -18,16 +18,30 @@ class RowIdTracker {
  */
 export function getRelatedRows(docActions: DocAction[]): ReadonlyArray<readonly [string, Set<number>]> {
   // Relate tableIds for tables with what they were before the actions, if renamed.
-  const tableIds = new Map<string, string>();
+  const tableIds = new Map<string, string>();      // key is current tableId
-  const rowIds = new Map<string, RowIdTracker>();
+  const rowIds = new Map<string, RowIdTracker>();  // key is pre-existing tableId
+  const addedTables = new Set<string>();  // track newly added tables to ignore; key is current tableId
   for (const docAction of docActions) {
     const currentTableId = getTableId(docAction);
     const tableId = tableIds.get(currentTableId) || currentTableId;
     if (docAction[0] === 'RenameTable') {
+      if (addedTables.has(currentTableId)) {
+        addedTables.delete(currentTableId);
+        addedTables.add(docAction[2])
+        continue;
+      }
       tableIds.delete(currentTableId);
       tableIds.set(docAction[2], tableId);
       continue;
     }
+    if (docAction[0] === 'AddTable') {
+      addedTables.add(currentTableId);
+    }
+    if (docAction[0] === 'RemoveTable') {
+      addedTables.delete(currentTableId);
+      continue;
+    }
+    if (addedTables.has(currentTableId)) { continue; }
 
     // tableId will now be that prior to docActions, regardless of renames.
     const tracker = getSetMapValue(rowIds, tableId, () => new RowIdTracker());

app/server/lib/Sharing.ts

@@ -213,7 +213,7 @@ export class Sharing {
       info.linkId = docSession.linkId;
     }
 
-    const {sandboxActionBundle, undo, docActions} =
+    const {sandboxActionBundle, undo, accessControl} =
       await this._modificationLock.runExclusive(() => this._applyActionsToDataEngine(docSession, userActions));
 
     // A trivial action does not merit allocating an actionNum,
@@ -285,13 +285,10 @@ export class Sharing {
       internal: isCalculate,
     });
     try {
-      await this._activeDoc.appliedActions(docActions, undo);
+      await accessControl.appliedBundle();
-      await this._activeDoc.broadcastDocUpdate(client || null, 'docUserAction', {
+      await accessControl.sendDocUpdateForBundle(actionGroup);
-        actionGroup,
-        docActions,
-      });
     } finally {
-      await this._activeDoc.finishedActions();
+      await accessControl.finishedBundle();
     }
     if (docSession) {
       docSession.linkId = docSession.shouldBundleActions ? localActionBundle.actionNum : 0;
@@ -375,18 +372,18 @@ export class Sharing {
     const docActions = getEnvContent(sandboxActionBundle.stored).concat(
       getEnvContent(sandboxActionBundle.calc));
 
+    const accessControl = this._activeDoc.getGranularAccessForBundle(docSession || makeExceptionalDocSession('share'), docActions, undo, userActions);
     try {
       // TODO: see if any of the code paths that have no docSession are relevant outside
       // of tests.
-      await this._activeDoc.canApplyDocActions(docSession || makeExceptionalDocSession('share'),
+      await accessControl.canApplyBundle();
-                                               docActions, undo);
     } catch (e) {
       // should not commit.  Don't write to db.  Remove changes from sandbox.
       await this._activeDoc.applyActionsToDataEngine([['ApplyUndoActions', undo]]);
-      await this._activeDoc.finishedActions();
+      await accessControl.finishedBundle();
       throw e;
     }
-    return {sandboxActionBundle, undo, docActions};
+    return {sandboxActionBundle, undo, docActions, accessControl};
   }
 }
 

sandbox/grist/acl.py

@@ -4,7 +4,7 @@
 
 import json
 
-from acl_formula import parse_acl_grist_entities
+from acl_formula import parse_acl_grist_entities, parse_acl_formula_json
 import action_obj
 import logger
 import textbuilder
@@ -128,7 +128,9 @@ def prepare_acl_col_renames(docmodel, useractions, col_renames_dict):
         patches.append(patch)
 
       replacer = textbuilder.Replacer(textbuilder.Text(formula), patches)
-      rule_updates.append((rule_rec, {'aclFormula': replacer.get_text().encode('utf8')}))
+      txt = replacer.get_text().encode('utf8')
+      rule_updates.append((rule_rec, {'aclFormula': txt,
+                                      'aclFormulaParsed': parse_acl_formula_json(txt)}))
 
   def do_renames():
     useractions.doBulkUpdateFromPairs('_grist_ACLResources', resource_updates)