(core) control the distribution of attachment metadata

Summary:
for users who don't automatically have deep rights
to the document, provide them with attachment metadata only
for rows they have access to. This is a little tricky to
do efficiently. We provide attachment metadata when an
individual table is fetched, rather than on initial document
load, so we don't block that load on a full document scan.
We provide attachment metadata to a client when we see that
we are shipping rows mentioning particular attachments,
without making any effort to keep track of the metadata they
already have.

Test Plan: updated tests

Reviewers: dsagal, jarek

Reviewed By: dsagal, jarek

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

app/client/components/WidgetFrame.ts

@@ -306,9 +306,9 @@ export class GristDocAPIImpl implements GristDocAPI {
 
   public async listTables(): Promise<string[]> {
     // Could perhaps read tableIds from this.gristDoc.docModel.visibleTableIds.all()?
-    const tables = await this._doc.docComm.fetchTable('_grist_Tables');
+    const {tableData} = await this._doc.docComm.fetchTable('_grist_Tables');
     // Tables the user doesn't have access to are just blanked out.
-    return tables[3].tableId.filter(tableId => tableId !== '') as string[];
+    return tableData[3].tableId.filter(tableId => tableId !== '') as string[];
   }
 
   public async fetchTable(tableId: string) {

app/common/ActiveDocAPI.ts

@@ -1,5 +1,5 @@
 import {ActionGroup} from 'app/common/ActionGroup';
-import {CellValue, TableDataAction, UserAction} from 'app/common/DocActions';
+import {BulkAddRecord, CellValue, TableDataAction, UserAction} from 'app/common/DocActions';
 import {FormulaProperties} from 'app/common/GranularAccessClause';
 import {UIRowId} from 'app/common/UIRowId';
 import {FetchUrlOptions, UploadResult} from 'app/common/uploads';
@@ -137,14 +137,30 @@ export interface QueryFilters {
 // - empty: value should be falsy (e.g. null) or an empty list, filters is ignored
 export type QueryOperation = "in" | "intersects" | "empty";
 
+/**
+ * Results of fetching a table. Includes the table data you would
+ * expect. May now also include attachment metadata referred to in the table
+ * data. Attachment data is expressed as a BulkAddRecord, since it is
+ * not a complete table, just selected rows. Attachment data is
+ * currently included in fetches when (1) granular access control is
+ * in effect, and (2) the user is neither an owner nor someone with
+ * read access to the entire document, and (3) there is an attachment
+ * column in the fetched table. This is exactly what the standard
+ * Grist client needs, but in future it might be desirable to give
+ * more control over this behavior.
+ */
+export interface TableFetchResult {
+  tableData: TableDataAction;
+  attachments?: BulkAddRecord;
+}
+
 /**
  * Response from useQuerySet(). A query returns data AND creates a subscription to receive
  * DocActions that affect this data. The querySubId field identifies this subscription, and must
  * be used in a disposeQuerySet() call to unsubscribe.
  */
-export interface QueryResult {
+export interface QueryResult extends TableFetchResult {
   querySubId: number;     // ID of the subscription, to use with disposeQuerySet.
-  tableData: TableDataAction;
 }
 
 /**
@@ -229,7 +245,7 @@ export interface ActiveDocAPI {
   /**
    * Fetches a particular table from the data engine to return to the client.
    */
-  fetchTable(tableId: string): Promise<TableDataAction>;
+  fetchTable(tableId: string): Promise<TableFetchResult>;
 
   /**
    * Fetches the generated Python code for this document. (TODO rename this misnomer.)

app/common/AttachmentColumns.ts

@@ -0,0 +1,82 @@
+import { AddRecord, BulkAddRecord, BulkRemoveRecord, BulkUpdateRecord,
+         getColIdsFromDocAction, getColValuesFromDocAction,
+         getTableId, RemoveRecord, ReplaceTableData, TableDataAction,
+         UpdateRecord } from 'app/common/DocActions';
+import { DocData } from 'app/common/DocData';
+import { isNumber } from 'app/common/gutil';
+
+/**
+ * Represent current attachment columns as a map from tableId to a set of
+ * colIds.
+ */
+export type AttachmentColumns = Map<string, Set<string>>;
+
+/**
+ * Enumerate attachment columns, represented as a map from tableId to
+ * a set of colIds.
+ */
+export function getAttachmentColumns(metaDocData: DocData): AttachmentColumns {
+  const tablesTable = metaDocData.getMetaTable('_grist_Tables');
+  const columnsTable = metaDocData.getMetaTable('_grist_Tables_column');
+  const attachmentColumns: Map<string, Set<string>> = new Map();
+  for (const column of columnsTable.filterRecords({type: 'Attachments'})) {
+    const table = tablesTable.getRecord(column.parentId);
+    const tableId = table?.tableId;
+    if (!tableId) {
+      /* should never happen */
+      throw new Error('table not found');
+    }
+    if (!attachmentColumns.has(tableId)) {
+      attachmentColumns.set(tableId, new Set());
+    }
+    attachmentColumns.get(tableId)!.add(column.colId);
+  }
+  return attachmentColumns;
+}
+
+/**
+ * Get IDs of attachments that are present in attachment columns in an action.
+ */
+export function gatherAttachmentIds(
+  attachmentColumns: AttachmentColumns,
+  action: AddRecord | BulkAddRecord | UpdateRecord | BulkUpdateRecord |
+    RemoveRecord | BulkRemoveRecord | ReplaceTableData | TableDataAction
+): Set<number> {
+  const tableId = getTableId(action);
+  const attColumns = attachmentColumns.get(tableId);
+  const colIds = getColIdsFromDocAction(action) || [];
+  const attIds = new Set<number>();
+  if (!attColumns || !colIds.some(colId => attColumns.has(colId))) {
+    return attIds;
+  }
+  for (const colId of colIds) {
+    if (!attColumns.has(colId)) { continue; }
+    const values = getColValuesFromDocAction(action, colId);
+    if (!values) { continue; }
+    for (const v of values) {
+      // We expect an array. What should we do with other types?
+      // If we were confident no part of Grist would interpret non-array
+      // values as attachment ids, then we should let them be added, as
+      // part of Grist's spreadsheet-style willingness to allow invalid
+      // data. I decided to go ahead and require that numbers or number-like
+      // strings should be checked as if they were attachment ids, just in
+      // case. But if this proves awkward for someone, it could be reasonable
+      // to only check ids in an array after confirming Grist is strict in
+      // how it interprets material in attachment cells.
+      if (typeof v === 'number') {
+        attIds.add(v);
+      } else if (Array.isArray(v)) {
+        for (const p of v) {
+          if (typeof p === 'number') {
+            attIds.add(p);
+          }
+        }
+      } else if (typeof v === 'boolean' || v === null) {
+        // Nothing obvious to do here.
+      } else if (isNumber(v)) {
+        attIds.add(Math.round(parseFloat(v)));
+      }
+    }
+  }
+  return attIds;
+}

app/common/DocActions.ts

@@ -178,9 +178,12 @@ export function toTableDataAction(tableId: string, colValues: TableColValues): T
 
 // Convert from TableDataAction (used mainly by the sandbox) to TableColValues (used by DocStorage
 // and external APIs).
-export function fromTableDataAction(tableData: TableDataAction): TableColValues {
-  const rowIds: number[] = tableData[2];
-  const colValues: BulkColValues = tableData[3];
+// Also accepts a TableDataAction nested as a tableData member of a larger structure,
+// for convenience in dealing with the result of fetches.
+export function fromTableDataAction(tableData: TableDataAction|{tableData: TableDataAction}): TableColValues {
+  const data = ('tableData' in tableData) ? tableData.tableData : tableData;
+  const rowIds: number[] = data[2];
+  const colValues: BulkColValues = data[3];
   return {id: rowIds, ...colValues};
 }
 
@@ -203,3 +206,33 @@ export function getColValues(records: Partial<RowRecord>[]): BulkColValues {
   }
   return result;
 }
+
+/**
+ * Extract the col ids mentioned in a record-related DocAction as a list
+ * (even if the action is not a bulk action). Returns undefined if no col ids
+ * mentioned.
+ */
+export function getColIdsFromDocAction(docActions: RemoveRecord | BulkRemoveRecord | AddRecord |
+  BulkAddRecord | UpdateRecord | BulkUpdateRecord | ReplaceTableData |
+  TableDataAction): string[] | undefined {
+  if (docActions[3]) { return Object.keys(docActions[3]); }
+  return undefined;
+}
+
+/**
+ * Extract column values for a particular column as CellValue[] from a
+ * record-related DocAction. Undefined if absent.
+ */
+export function getColValuesFromDocAction(docAction: RemoveRecord | BulkRemoveRecord | AddRecord |
+  BulkAddRecord | UpdateRecord | BulkUpdateRecord | ReplaceTableData |
+  TableDataAction, colId: string): CellValue[]|undefined {
+  const colValues = docAction[3];
+  if (!colValues) { return undefined; }
+  const cellValues = colValues[colId];
+  if (!cellValues) { return undefined; }
+  if (Array.isArray(docAction[2])) {
+    return cellValues as CellValue[];
+  } else {
+    return [cellValues as CellValue];
+  }
+}

app/common/DocData.ts

@@ -9,24 +9,39 @@ import {schema, SchemaTypes} from 'app/common/schema';
 import fromPairs = require('lodash/fromPairs');
 import groupBy = require('lodash/groupBy');
 import {ActionDispatcher} from './ActionDispatcher';
+import {TableFetchResult} from './ActiveDocAPI';
 import {
   BulkColValues, ColInfo, ColInfoWithId, ColValues, DocAction,
   RowRecord, TableDataAction
 } from './DocActions';
 import {ColTypeMap, MetaRowRecord, MetaTableData, TableData} from './TableData';
 
-type FetchTableFunc = (tableId: string) => Promise<TableDataAction>;
+type FetchTableFunc = (tableId: string) => Promise<TableFetchResult>;
 
 export class DocData extends ActionDispatcher {
   private _tables: Map<string, TableData> = new Map();
 
+  private _fetchTableFunc: (tableId: string) => Promise<TableDataAction>;
+
   /**
    * If metaTableData is not supplied, then any tables needed should be loaded manually,
    * using syncTable(). All column types will be set to Any, which will affect default
    * values.
    */
-  constructor(private _fetchTableFunc: FetchTableFunc, metaTableData: {[tableId: string]: TableDataAction} | null) {
+  constructor(fetchTableFunc: FetchTableFunc, metaTableData: {[tableId: string]: TableDataAction} | null) {
     super();
+    // Wrap fetchTableFunc slightly to handle any extra attachment data that
+    // may come along for the ride.
+    this._fetchTableFunc = async (tableId: string) => {
+      const {tableData, attachments} = await fetchTableFunc(tableId);
+      if (attachments) {
+        // Back-end doesn't keep track of which attachments we already have,
+        // so there may be duplicates of rows we already have - but happily
+        // BulkAddRecord overwrites duplicates now.
+        this.receiveAction(attachments);
+      }
+      return tableData;
+    };
     if (metaTableData === null) { return; }
     // Create all meta tables, and populate data we already have.
     for (const tableId in schema) {

app/common/TableData.ts

@@ -2,7 +2,8 @@
  * TableData maintains a single table's data.
  */
 import {ActionDispatcher} from 'app/common/ActionDispatcher';
-import {BulkColValues, CellValue, ColInfo, ColInfoWithId, ColValues, DocAction,
+import {
+  BulkAddRecord, BulkColValues, CellValue, ColInfo, ColInfoWithId, ColValues, DocAction,
         isSchemaAction, ReplaceTableData, RowRecord, TableDataAction} from 'app/common/DocActions';
 import {getDefaultForType} from 'app/common/gristTypes';
 import {arrayRemove, arraySplice, getDistinctValues} from 'app/common/gutil';
@@ -250,16 +251,40 @@ export class TableData extends ActionDispatcher implements SkippableRows {
 
   /**
    * Return data in TableDataAction form ['TableData', tableId, [...rowIds], {...}]
+   * Optionally takes a list of row ids to return data from. If a row id is
+   * not actually present in the table, a row of nulls will be returned for it.
    */
-  public getTableDataAction(): TableDataAction {
-    const rowIds = this.getRowIds();
+  public getTableDataAction(desiredRowIds?: number[]): TableDataAction {
+    const rowIds = desiredRowIds || this.getRowIds();
+    let bulkColValues: {[colId: string]: CellValue[]};
+    if (desiredRowIds) {
+      const len = rowIds.length;
+      bulkColValues = {};
+      for (const colId of this.getColIds()) { bulkColValues[colId] = Array(len); }
+      for (let i = 0; i < len; i++) {
+        const index = this._rowMap.get(rowIds[i]);
+        for (const {colId, values} of this._colArray) {
+          const value = (index === undefined) ? null : values[index];
+          bulkColValues[colId][i] = value;
+        }
+      }
+    } else {
+      bulkColValues = fromPairs(
+        this.getColIds()
+          .filter(colId => colId !== 'id')
+          .map(colId => [colId, this.getColValues(colId)! as CellValue[]]));
+    }
     return ['TableData',
             this.tableId,
             rowIds as number[],
-            fromPairs(
-              this.getColIds()
-                .filter(colId => colId !== 'id')
-                .map(colId => [colId, this.getColValues(colId)! as CellValue[]]))];
+            bulkColValues];
+  }
+
+  public getBulkAddRecord(desiredRowIds?: number[]): BulkAddRecord {
+    const tableData = this.getTableDataAction(desiredRowIds?.sort((a, b) => a - b));
+    return [
+      'BulkAddRecord', tableData[1], tableData[2], tableData[3],
+    ];
   }
 
   /**
@@ -357,6 +382,13 @@ export class TableData extends ActionDispatcher implements SkippableRows {
   // ---- The following methods implement ActionDispatcher interface ----
 
   protected onAddRecord(action: DocAction, tableId: string, rowId: number, colValues: ColValues): void {
+    if (this._rowMap.get(rowId) !== undefined) {
+      // If adding a record that already exists, act like an update.
+      // We rely on this behavior for distributing attachment
+      // metadata.
+      this.onUpdateRecord(action, tableId, rowId, colValues);
+      return;
+    }
     const index: number = this._rowIdCol.length;
     this._rowMap.set(rowId, index);
     this._rowIdCol[index] = rowId;
@@ -366,14 +398,28 @@ export class TableData extends ActionDispatcher implements SkippableRows {
   }
 
   protected onBulkAddRecord(action: DocAction, tableId: string, rowIds: number[], colValues: BulkColValues): void {
-    const index: number = this._rowIdCol.length;
+    let destIndex: number = this._rowIdCol.length;
     for (let i = 0; i < rowIds.length; i++) {
-      this._rowMap.set(rowIds[i], index + i);
-      this._rowIdCol[index + i] = rowIds[i];
-    }
-    for (const {colId, defl, values} of this._colArray) {
-      for (let i = 0; i < rowIds.length; i++) {
-        values[index + i] = colValues.hasOwnProperty(colId) ? colValues[colId][i] : defl;
+      const srcIndex = this._rowMap.get(rowIds[i]);
+      if (srcIndex !== undefined) {
+        // If adding a record that already exists, act like an update.
+        // We rely on this behavior for distributing attachment
+        // metadata.
+        for (const colId in colValues) {
+          if (colValues.hasOwnProperty(colId)) {
+            const colData = this._columns.get(colId);
+            if (colData) {
+              colData.values[srcIndex] = colValues[colId][i];
+            }
+          }
+        }
+      } else {
+        this._rowMap.set(rowIds[i], destIndex);
+        this._rowIdCol[destIndex] = rowIds[i];
+        for (const {colId, defl, values} of this._colArray) {
+          values[destIndex] = colValues.hasOwnProperty(colId) ? colValues[colId][i] : defl;
+        }
+        destIndex++;
       }
     }
   }

app/server/lib/ActiveDoc.ts

@@ -29,16 +29,20 @@ import {
   PermissionDataWithExtraUsers,
   QueryResult,
   ServerQuery,
+  TableFetchResult,
   TransformRule
 } from 'app/common/ActiveDocAPI';
 import {ApiError} from 'app/common/ApiError';
 import {mapGetOrSet, MapWithTTL} from 'app/common/AsyncCreate';
+import {AttachmentColumns, gatherAttachmentIds, getAttachmentColumns} from 'app/common/AttachmentColumns';
 import {
+  BulkAddRecord,
   BulkRemoveRecord,
   BulkUpdateRecord,
   CellValue,
   DocAction,
   getTableId,
+  isSchemaAction,
   TableDataAction,
   TableRecordValue,
   toTableDataAction,
@@ -213,6 +217,8 @@ export class ActiveDoc extends EventEmitter {
   private _gracePeriodStart: Date|null = null;
   private _isForkOrSnapshot: boolean = false;
   private _onlyAllowMetaDataActionsOnDb: boolean = false;
+  // Cache of which columns are attachment columns.
+  private _attachmentColumns?: AttachmentColumns;
 
   // Client watching for 'product changed' event published by Billing to update usage
   private _redisSubscriber?: RedisClient;
@@ -970,7 +976,7 @@ export class ActiveDoc extends EventEmitter {
    *      form of the form ["TableData", table_id, row_ids, column_values].
    */
   public async fetchTable(docSession: OptDocSession, tableId: string,
-                          waitForFormulas: boolean = false): Promise<TableDataAction> {
+                          waitForFormulas: boolean = false): Promise<TableFetchResult> {
     return this.fetchQuery(docSession, {tableId, filters: {}}, waitForFormulas);
   }
 
@@ -982,7 +988,7 @@ export class ActiveDoc extends EventEmitter {
    * special "pending" values may be returned.
    */
   public async fetchQuery(docSession: OptDocSession, query: ServerQuery,
-                          waitForFormulas: boolean = false): Promise<TableDataAction> {
+                          waitForFormulas: boolean = false): Promise<TableFetchResult> {
     this._inactivityTimer.ping();     // The doc is in active use; ping it to stay open longer.
 
     // If user does not have rights to access what this query is asking for, fail.
@@ -999,8 +1005,8 @@ export class ActiveDoc extends EventEmitter {
       // table.  So we pick out the table we want from fetchMetaTables (which has applied
       // filtering).
       const tables = await this.fetchMetaTables(docSession);
-      const table = tables[query.tableId];
-      if (table) { return table; }
+      const tableData = tables[query.tableId];
+      if (tableData) { return {tableData}; }
       // If table not found, continue, to give a consistent error for a table not found.
     }
 
@@ -1036,9 +1042,23 @@ export class ActiveDoc extends EventEmitter {
       data = cloneDeep(data!);  // Clone since underlying fetch may be cached and shared.
       await this._granularAccess.filterData(docSession, data);
     }
+
+    // Consider whether we need to add attachment metadata.
+    // TODO: it might be desirable to always send attachment data, or allow
+    // this to be an option in api calls related to fetching.
+    let attachments: BulkAddRecord | undefined;
+    const attachmentColumns = this._getCachedAttachmentColumns();
+    if (attachmentColumns?.size && await this._granularAccess.needAttachmentControl(docSession)) {
+      const attIds = gatherAttachmentIds(attachmentColumns, data!);
+      if (attIds.size > 0) {
+        attachments = this.docData!.getMetaTable('_grist_Attachments')
+          .getBulkAddRecord([...attIds]);
+      }
+    }
+
     this._log.info(docSession, "fetchQuery -> %d rows, cols: %s",
              data![2].length, Object.keys(data![3]).join(", "));
-    return data!;
+    return {tableData: data!, ...(attachments && {attachments})};
   }
 
   /**
@@ -1071,8 +1091,8 @@ export class ActiveDoc extends EventEmitter {
     // - Subscription should not be affected by renames (so don't hold on to query/tableId/colIds)
     // - Table/column deletion should make subscription inactive, and unsubscribing an inactive
     //   subscription should not produce an error.
-    const tableData: TableDataAction = await this.fetchQuery(docSession, query);
-    return {querySubId: 0, tableData};
+    const tableFetchResult = await this.fetchQuery(docSession, query);
+    return {querySubId: 0, ...tableFetchResult};
   }
 
   /**
@@ -1241,6 +1261,9 @@ export class ActiveDoc extends EventEmitter {
       await this.docStorage.updateIndexes(indexes);
       // TODO: should probably add indexes for user attribute tables.
     }
+    if (docActions.some(docAction => isSchemaAction(docAction))) {
+      this._attachmentColumns = undefined;
+    }
   }
 
   /**
@@ -2330,6 +2353,14 @@ export class ActiveDoc extends EventEmitter {
     await this._updateDocUsage({attachmentsSizeBytes}, options);
     return attachmentsSizeBytes;
   }
+
+  private _getCachedAttachmentColumns(): AttachmentColumns {
+    if (!this.docData) { return new Map(); }
+    if (!this._attachmentColumns) {
+      this._attachmentColumns = getAttachmentColumns(this.docData);
+    }
+    return this._attachmentColumns;
+  }
 }
 
 // Helper to initialize a sandbox action bundle with no values.

app/server/lib/DocApi.ts

@@ -164,7 +164,7 @@ export class DocWorkerApi {
       }
       const tableId = optTableId || req.params.tableId;
       const session = docSessionFromRequest(req);
-      const tableData = await handleSandboxError(tableId, [], activeDoc.fetchQuery(
+      const {tableData} = await handleSandboxError(tableId, [], activeDoc.fetchQuery(
         session, {tableId, filters}, !immediate));
       // For metaTables we don't need to specify columns, search will infer it from the sort expression.
       const isMetaTable = tableId.startsWith('_grist');

app/server/lib/Export.ts

@@ -209,9 +209,9 @@ export async function exportTable(
   }).filter(tc => tc !== emptyCol);
 
   // fetch actual data
-  const data = await activeDoc.fetchTable(docSessionFromRequest(req as RequestWithLogin), table.tableId, true);
-  const rowIds = data[2];
-  const dataByColId = data[3];
+  const {tableData} = await activeDoc.fetchTable(docSessionFromRequest(req as RequestWithLogin), table.tableId, true);
+  const rowIds = tableData[2];
+  const dataByColId = tableData[3];
   // sort rows
   const getters = new ServerColumnGetters(rowIds, dataByColId, columns);
   // create cell accessors
@@ -311,9 +311,9 @@ export async function exportSection(
   });
 
   // fetch actual data
-  const data = await activeDoc.fetchTable(docSessionFromRequest(req as RequestWithLogin), table.tableId, true);
-  let rowIds = data[2];
-  const dataByColId = data[3];
+  const {tableData} = await activeDoc.fetchTable(docSessionFromRequest(req as RequestWithLogin), table.tableId, true);
+  let rowIds = tableData[2];
+  const dataByColId = tableData[3];
   // sort rows
   const getters = new ServerColumnGetters(rowIds, dataByColId, columns);
   const sorter = new SortFunc(getters);

app/server/lib/GranularAccess.ts

@@ -17,9 +17,10 @@ import {
   isBulkUpdateRecord,
   isUpdateRecord,
 } from 'app/common/DocActions';
+import { AttachmentColumns, gatherAttachmentIds, getAttachmentColumns } from 'app/common/AttachmentColumns';
 import { RemoveRecord, ReplaceTableData, UpdateRecord } from 'app/common/DocActions';
 import { CellValue, ColValues, DocAction, getTableId, isSchemaAction } from 'app/common/DocActions';
-import { TableDataAction, UserAction } from 'app/common/DocActions';
+import { getColIdsFromDocAction, TableDataAction, UserAction } from 'app/common/DocActions';
 import { DocData } from 'app/common/DocData';
 import { UserOverride } from 'app/common/DocListAPI';
 import { DocUsageSummary, FilteredDocUsageSummary } from 'app/common/DocUsage';
@@ -28,7 +29,7 @@ import { ErrorWithCode } from 'app/common/ErrorWithCode';
 import { AclMatchInput, InfoEditor, InfoView } from 'app/common/GranularAccessClause';
 import { UserInfo } from 'app/common/GranularAccessClause';
 import * as gristTypes from 'app/common/gristTypes';
-import { getSetMapValue, isNonNullish, isNumber, pruneArray } from 'app/common/gutil';
+import { getSetMapValue, isNonNullish, pruneArray } from 'app/common/gutil';
 import { MetaRowRecord, SingleCell } from 'app/common/TableData';
 import { canEdit, canView, isValidRole, Role } from 'app/common/roles';
 import { FullUser, UserAccessData } from 'app/common/UserAPI';
@@ -44,12 +45,11 @@ import { IPermissionInfo, MixedPermissionSetWithContext,
          PermissionInfo, PermissionSetWithContext } from 'app/server/lib/PermissionInfo';
 import { TablePermissionSetWithContext } from 'app/server/lib/PermissionInfo';
 import { integerParam } from 'app/server/lib/requestUtils';
-import { getColIdsFromDocAction, getColValuesFromDocAction, getRelatedRows,
-         getRowIdsFromDocAction } from 'app/server/lib/RowAccess';
+import { getRelatedRows, getRowIdsFromDocAction } from 'app/server/lib/RowAccess';
 import cloneDeep = require('lodash/cloneDeep');
 import fromPairs = require('lodash/fromPairs');
-import memoize = require('lodash/memoize');
 import get = require('lodash/get');
+import memoize = require('lodash/memoize');
 
 // tslint:disable:no-bitwise
 
@@ -597,9 +597,11 @@ export class GranularAccess implements GranularAccessForBundle {
 
     const actions = await Promise.all(
       docActions.map((action, actionIdx) => this._filterOutgoingDocAction({docSession, action, actionIdx})));
-    const result = ([] as DocAction[]).concat(...actions);
+    let result = ([] as ActionCursor[]).concat(...actions);
+    result = await this._filterOutgoingAttachments(result);
 
-    return await this._filterOutgoingCellInfo(docSession, docActions, result);
+    return await this._filterOutgoingCellInfo(docSession, docActions,
+                                              result.map(a => a.action));
   }
 
 
@@ -820,8 +822,12 @@ export class GranularAccess implements GranularAccessForBundle {
   }
 
   /**
-   * An odd little right for findColFromValues and autocomplete.  Allow if user can read
-   * all data, or is an owner.  Might be worth making a special permission.
+   * Allow if user can read all data, or is an owner.
+   * Might be worth making a special permission.
+   * At the time of writing, used for:
+   *   - findColFromValues
+   *   - autocomplete
+   *   - unfiltered access to attachment metadata
    */
   public async canScanData(docSession: OptDocSession): Promise<boolean> {
     return await this.isOwner(docSession) || await this.canReadEverything(docSession);
@@ -903,6 +909,17 @@ export class GranularAccess implements GranularAccessForBundle {
     for (const tableId of STRUCTURAL_TABLES) {
       censor.apply(tables[tableId]);
     }
+    if (await this.needAttachmentControl(docSession)) {
+      // Attachments? No attachments here (whistles innocently).
+      // Computing which attachments user has access to would require
+      // looking at entire document, which we don't want to do. So instead
+      // we'll be sending this info on a need-to-know basis later.
+      const attachments = tables['_grist_Attachments'];
+      attachments[2] = [];
+      Object.values(attachments[3]).forEach(values => {
+        values.length = 0;
+      });
+    }
     return tables;
   }
 
@@ -1083,7 +1100,12 @@ export class GranularAccess implements GranularAccessForBundle {
     rows.delete('_grist_Cells');
     // Populate a minimal in-memory version of the database with these rows.
     const docData = new DocData(
-      (tableId) => this._fetchQueryFromDB({tableId, filters: {id: [...rows.get(tableId)!]}}), {
+      async (tableId) => {
+        return {
+          tableData: await this._fetchQueryFromDB(
+            {tableId, filters: {id: [...rows.get(tableId)!]}})
+        };
+      }, {
         _grist_Cells: this._docData.getMetaTable('_grist_Cells')!.getTableDataAction(),
         // We need some basic table information to translate numeric ids to string ids (refs to ids).
         _grist_Tables: this._docData.getMetaTable('_grist_Tables')!.getTableDataAction(),
@@ -1095,6 +1117,11 @@ export class GranularAccess implements GranularAccessForBundle {
     return docData;
   }
 
+  // Return true if attachment info must be sent on a need-to-know basis.
+  public async needAttachmentControl(docSession: OptDocSession) {
+    return !await this.canScanData(docSession);
+  }
+
   /**
    * An optimization to catch obvious access problems for simple data
    * actions (such as UpdateRecord, BulkAddRecord, etc) early. Checks
@@ -1971,7 +1998,11 @@ export class GranularAccess implements GranularAccessForBundle {
     const rows = new Map(getRelatedRows(applied ? [...undo].reverse() : docActions));
     // Populate a minimal in-memory version of the database with these rows.
     const docData = new DocData(
-      (tableId) => this._fetchQueryFromDB({tableId, filters: {id: [...rows.get(tableId)!]}}),
+      async (tableId) => {
+        return {
+          tableData: await this._fetchQueryFromDB({tableId, filters: {id: [...rows.get(tableId)!]}})
+        };
+      },
       null,
     );
     // Load pre-existing rows touched by the bundle.
@@ -2015,14 +2046,14 @@ export class GranularAccess implements GranularAccessForBundle {
     if (!needMeta) {
       // Sometimes, the intermediate states are trivial.
       // TODO: look into whether it would be worth caching attachment columns.
-      const attachmentColumns = this._getAttachmentColumns(this._docData);
+      const attachmentColumns = getAttachmentColumns(this._docData);
       return docActions.map(action => ({action, attachmentColumns}));
     }
     const metaDocData = new DocData(
       async (tableId) => {
         const result = this._docData.getTable(tableId)?.getTableDataAction();
         if (!result) { throw new Error('surprising load'); }
-        return result;
+        return {tableData: result};
       },
       null,
     );
@@ -2067,33 +2098,12 @@ export class GranularAccess implements GranularAccessForBundle {
         replaceRuler = false;
       }
       step.ruler = ruler;
-      step.attachmentColumns = this._getAttachmentColumns(metaDocData);
+      step.attachmentColumns = getAttachmentColumns(metaDocData);
       steps.push(step);
     }
     return steps;
   }
 
-  /**
-   * Enumerate attachment columns, represented as a map from tableId to
-   * a set of colIds.
-   */
-  private _getAttachmentColumns(metaDocData: DocData): Map<string, Set<string>> {
-    const tablesTable = metaDocData.getMetaTable('_grist_Tables');
-    const columnsTable = metaDocData.getMetaTable('_grist_Tables_column');
-    const attachmentColumns: Map<string, Set<string>> = new Map();
-    for (const col of columnsTable.filterRecords({type: 'Attachments'})) {
-      const table = tablesTable.getRecord(col.parentId);
-      const tableId = table?.tableId;
-      if (!tableId) { throw new Error('table not found'); /* should not happen */ }
-      const colId = col.colId;
-      if (!attachmentColumns.has(tableId)) {
-        attachmentColumns.set(tableId, new Set());
-      }
-      attachmentColumns.get(tableId)!.add(colId);
-    }
-    return attachmentColumns;
-  }
-
   /**
    * Return any permitted parts of an action.  A completely forbidden
    * action results in an empty list.  Forbidden columns and rows will
@@ -2163,7 +2173,7 @@ export class GranularAccess implements GranularAccessForBundle {
    * TODO: I think that column rules controlling READ access using rec are not fully supported
    * yet.  They work on first load, but if READ access is lost/gained updates won't be made.
    */
-  private async _filterOutgoingDocAction(cursor: ActionCursor): Promise<DocAction[]> {
+  private async _filterOutgoingDocAction(cursor: ActionCursor): Promise<ActionCursor[]> {
     const {action} = cursor;
     const tableId = getTableId(action);
 
@@ -2200,7 +2210,7 @@ export class GranularAccess implements GranularAccessForBundle {
         secondPass.push(act);
       }
     }
-    return secondPass;
+    return secondPass.map(act => ({ ...cursor, action: act }));
   }
 
   private async _filterOutgoingStructuralTables(cursor: ActionCursor, act: DataAction, results: DocAction[]) {
@@ -2270,61 +2280,89 @@ export class GranularAccess implements GranularAccessForBundle {
    * has the right to access any attachments mentioned.
    */
   private async _checkIncomingAttachmentChanges(cursor: ActionCursor): Promise<void> {
+    const {docSession} = cursor;
+    const attIds = await this._gatherAttachmentChanges(cursor);
+    for (const attId of attIds) {
+      if (!await this.isAttachmentUploadedByUser(docSession, attId) &&
+        !await this.findAttachmentCellForUser(docSession, attId)) {
+        throw new ErrorWithCode('ACL_DENY', 'Cannot access attachment', {
+          status: 403,
+        });
+      }
+    }
+  }
+
+  /**
+   * If user doesn't have sufficient rights, rewrite any attachment information
+   * as follows:
+   *   - Remove data actions (other than [Bulk]RemoveRecord) on the _grist_Attachments table
+   *   - Gather any attachment ids mentioned in data actions
+   *   - Prepend a BulkAddRecord for _grist_Attachments giving metadata for the attachments
+   * This will result in metadata being sent to clients more than necessary,
+   * but saves us keeping track of which clients already know about which
+   * attachments.
+   * We don't make any particular effort to retract attachment metadata from
+   * clients if they lose access to it later. They won't have access to the
+   * content of the attachment, and will lose metadata on a document reload.
+   */
+  private async _filterOutgoingAttachments(cursors: ActionCursor[]) {
+    if (cursors.length === 0) { return []; }
+    const docSession = cursors[0].docSession;
+    if (!await this.needAttachmentControl(docSession)) {
+      return cursors;
+    }
+    const result = [] as ActionCursor[];
+    const attIds = new Set<number>();
+    for (const cursor of cursors) {
+      const changes = await this._gatherAttachmentChanges(cursor);
+      // We assume here that ACL rules were already applied and columns were
+      // either removed or censored.
+      // Gather all attachment ids stored in user tables.
+      for (const attId of changes) {
+        attIds.add(attId);
+      }
+      const {action} = cursor;
+      // Remove any additions or updates to the _grist_Attachments table.
+      if (!isDataAction(action) || isRemoveRecordAction(action) || getTableId(action) !== '_grist_Attachments') {
+        result.push(cursor);
+      }
+    }
+    // We removed all actions that created attachments, now send all attachments metadata
+    // we currently have that are related to actions being broadcast.
+    if (attIds.size > 0) {
+      const act = this._docData.getMetaTable('_grist_Attachments')
+        .getBulkAddRecord([...attIds]);
+      result.unshift({
+        action: act,
+        docSession,
+        // For access control purposes, this new action will be under the
+        // same access rules as the first DocAction.
+        actionIdx: cursors[0].actionIdx,
+      });
+    }
+    return result;
+  }
+
+  private async _gatherAttachmentChanges(cursor: ActionCursor): Promise<Set<number>> {
+    const empty = new Set<number>();
     const options = this._activeBundle?.options;
     if (options?.fromOwnHistory && options.oldestSource &&
       Date.now() - options.oldestSource < HISTORICAL_ATTACHMENT_OWNERSHIP_PERIOD) {
-      return;
+      return empty;
     }
     const {action, docSession} = cursor;
-    if (!isDataAction(action)) { return; }
-    if (isRemoveRecordAction(action)) { return; }
+    if (!isDataAction(action)) { return empty; }
+    if (isRemoveRecordAction(action)) { return empty; }
     const tableId = getTableId(action);
     const step = await this._getMetaStep(cursor);
     const attachmentColumns = step.attachmentColumns;
-    if (!attachmentColumns) { return; }
+    if (!attachmentColumns) { return empty; }
     const ac = attachmentColumns.get(tableId);
-    if (!ac) { return; }
-    const colIds = getColIdsFromDocAction(action);
-    if (!colIds.some(colId => ac.has(colId))) { return; }
-    if (await this.isOwner(docSession) || await this.canReadEverything(docSession)) { return; }
-    const attIds = new Set<number>();
-    for (const colId of colIds) {
-      if (!ac.has(colId)) { continue; }
-      const values = getColValuesFromDocAction(action, colId);
-      if (!values) { continue; }
-      for (const v of values) {
-        // We expect an array. What should we do with other types?
-        // If we were confident no part of Grist would interpret non-array
-        // values as attachment ids, then we should let them be added, as
-        // part of Grist's spreadsheet-style willingness to allow invalid
-        // data. I decided to go ahead and require that numbers or number-like
-        // strings should be checked as if they were attachment ids, just in
-        // case. But if this proves awkward for someone, it could be reasonable
-        // to only check ids in an array after confirming Grist is strict in
-        // how it interprets material in attachment cells.
-        if (typeof v === 'number') {
-          attIds.add(v);
-        } else if (Array.isArray(v)) {
-          for (const p of v) {
-            if (typeof p === 'number') {
-              attIds.add(p);
-            }
-          }
-        } else if (typeof v === 'boolean' || v === null) {
-          // Nothing obvious to do here.
-        } else if (isNumber(v)) {
-          attIds.add(Math.round(parseFloat(v)));
-        }
-      }
-    }
-    for (const attId of attIds) {
-      if (!await this.isAttachmentUploadedByUser(docSession, attId) &&
-        !await this.findAttachmentCellForUser(docSession, attId)) {
-        throw new ErrorWithCode('ACL_DENY', 'Cannot access attachment', {
-          status: 403,
-        });
-      }
-    }
+    if (!ac) { return empty; }
+    const colIds = getColIdsFromDocAction(action) || [];
+    if (!colIds.some(colId => ac.has(colId))) { return empty; }
+    if (!await this.needAttachmentControl(docSession)) { return empty; }
+    return gatherAttachmentIds(attachmentColumns, action);
   }
 
   private async _getRuler(cursor: ActionCursor) {
@@ -2563,7 +2601,7 @@ export interface MetaStep {
   metaBefore?: {[key: string]: TableDataAction};  // cached structural metadata before action
   metaAfter?: {[key: string]: TableDataAction};   // cached structural metadata after action
   ruler?: Ruler;                          // rules at this step
-  attachmentColumns?: Map<string, Set<string>>;        // attachment columns after this step
+  attachmentColumns?: AttachmentColumns;        // attachment columns after this step
 }
 
 /**
@@ -2572,7 +2610,10 @@ export interface MetaStep {
 interface ActionCursor {
   action: DocAction;
   docSession: OptDocSession;
-  actionIdx: number|null;
+  actionIdx: number|null;     // an index into where we are within the original
+                              // DocActions, for access control purposes.
+                              // Used for referencing a cache of intermediate
+                              // access control state.
 }
 
 /**

app/server/lib/RowAccess.ts

@@ -1,5 +1,5 @@
 import { AddRecord, BulkAddRecord, BulkRemoveRecord, BulkUpdateRecord,
-         CellValue, DocAction, getTableId, RemoveRecord, ReplaceTableData,
+         DocAction, getTableId, RemoveRecord, ReplaceTableData,
          TableDataAction, UpdateRecord } from "app/common/DocActions";
 import { getSetMapValue } from "app/common/gutil";
 
@@ -77,33 +77,3 @@ export function getRowIdsFromDocAction(docActions: RemoveRecord | BulkRemoveReco
   const ids = docActions[2];
   return (typeof ids === 'number') ? [ids] : ids;
 }
-
-/**
- * Tiny helper to get the col ids mentioned in a record-related DocAction as a list
- * (even if the action is not a bulk action). When the action touches the whole row,
- * it returns ["*"].
- */
-export function getColIdsFromDocAction(docActions: RemoveRecord | BulkRemoveRecord | AddRecord |
-  BulkAddRecord | UpdateRecord | BulkUpdateRecord | ReplaceTableData |
-  TableDataAction) {
-  if (docActions[3]) { return Object.keys(docActions[3]); }
-  return ['*'];
-}
-
-/**
- * Extract column values for a particular column as CellValue[] from a
- * record-related DocAction. Undefined if absent.
- */
-export function getColValuesFromDocAction(docAction: RemoveRecord | BulkRemoveRecord | AddRecord |
-  BulkAddRecord | UpdateRecord | BulkUpdateRecord | ReplaceTableData |
-  TableDataAction, colId: string): CellValue[]|undefined {
-  const colValues = docAction[3];
-  if (!colValues) { return undefined; }
-  const cellValues = colValues[colId];
-  if (!cellValues) { return undefined; }
-  if (Array.isArray(docAction[2])) {
-    return cellValues as CellValue[];
-  } else {
-    return [cellValues as CellValue];
-  }
-}

app/server/lib/Triggers.ts

@@ -236,7 +236,8 @@ export class DocTriggers {
 
         // Fetch the modified records in full so they can be sent in webhooks
         // They will also be used to check if the record is ready
-        const tableDataAction = this._activeDoc.fetchQuery(docSession, {tableId, filters});
+        const tableDataAction = this._activeDoc.fetchQuery(docSession, {tableId, filters})
+          .then(tableFetchResult => tableFetchResult.tableData);
         tasks.push({tableDelta, triggers, tableDataAction, recordDeltas});
       }
     }

sandbox/gvisor/run.py

@@ -81,6 +81,15 @@ settings = {
   "ociVersion": "1.0.0",
   "process": {
     "terminal": include_bash,
+    # Match current user id, for convenience with mounts. For some versions of
+    # gvisor, default behavior may be better - if you see "access denied" problems
+    # during imports, try commenting this section out. We could make imports work
+    # for any version of gvisor by setting mode when using tmp.dir to allow
+    # others to list directory contents.
+    "user": {
+      "uid": os.getuid(),
+      "gid": 0
+    },
     "args": cmd_args,
     "env": env,
     "cwd": "/"
@@ -112,18 +121,6 @@ settings = {
     ]
   }
 }
-
-if not os.environ.get('GVISOR_USE_DEFAULT_USER'):
-  # Match current user id, for convenience with mounts. For some versions of
-  # gvisor, default behavior may be better - if you see "access denied" problems
-  # during imports, try setting GVISOR_USE_DEFAULT_USER. We could make imports work
-  # for any version of gvisor by setting mode when using tmp.dir to allow
-  # others to list directory contents.
-  settings['process']['user'] = {
-    "uid": os.getuid(),
-    "gid": 0
-  }
-
 memory_limit = os.environ.get('GVISOR_LIMIT_MEMORY')
 if memory_limit:
   settings['process']['rlimits'] = [

test/server/gristClient.ts

@@ -1,4 +1,6 @@
 import { DocAction } from 'app/common/DocActions';
+import { DocData } from 'app/common/DocData';
+import { SchemaTypes } from 'app/common/schema';
 import { FlexServer } from 'app/server/lib/FlexServer';
 import axios from 'axios';
 import pick = require('lodash/pick');
@@ -28,6 +30,7 @@ export class GristClient {
 
   private _requestId: number = 0;
   private _pending: Array<GristResponse|GristMessage> = [];
+  private _docData?: DocData;  // accumulate tabular info like a real client.
   private _consumer: () => void;
   private _ignoreTrivialActions: boolean = false;
 
@@ -41,6 +44,17 @@ export class GristClient {
         return;
       }
       this._pending.push(msg);
+      if (msg.data?.doc) {
+        this._docData = new DocData(() => {
+          throw new Error('no fetches');
+        }, msg.data.doc);
+      }
+      if (this._docData && msg.type === 'docUserAction') {
+        const docActions = msg.data?.docActions || [];
+        for (const docAction of docActions) {
+          this._docData.receiveAction(docAction);
+        }
+      }
       if (this._consumer) { this._consumer(); }
     };
   }
@@ -65,6 +79,15 @@ export class GristClient {
     return this._pending.length;
   }
 
+  public get docData() {
+    if (!this._docData) { throw new Error('no DocData'); }
+    return this._docData;
+  }
+
+  public getMetaRecords(tableId: keyof SchemaTypes) {
+    return this.docData.getMetaTable(tableId).getRecords();
+  }
+
   public async read(): Promise<any> {
     for (;;) {
       if (this._pending.length) {
@@ -97,6 +120,11 @@ export class GristClient {
     }
   }
 
+  public waitForServer() {
+    // send an arbitrary failing message and wait for response.
+    return this.send('ping');
+  }
+
   // Helper to read the next docUserAction ignoring anything else (e.g. a duplicate clientConnect).
   public async readDocUserAction(): Promise<DocAction[]> {
     while (true) {    // eslint-disable-line no-constant-condition