From 472a9a186ec0b5b87597e04011ffa7400948ba9a Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 21 Dec 2022 11:40:00 -0500 Subject: [PATCH] (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 | 4 +- app/common/ActiveDocAPI.ts | 24 ++- app/common/AttachmentColumns.ts | 82 +++++++++++ app/common/DocActions.ts | 39 ++++- app/common/DocData.ts | 19 ++- app/common/TableData.ts | 74 ++++++++-- app/server/lib/ActiveDoc.ts | 45 +++++- app/server/lib/DocApi.ts | 2 +- app/server/lib/Export.ts | 12 +- app/server/lib/GranularAccess.ts | 211 ++++++++++++++++----------- app/server/lib/RowAccess.ts | 32 +--- app/server/lib/Triggers.ts | 3 +- sandbox/gvisor/run.py | 21 ++- test/server/gristClient.ts | 28 ++++ 14 files changed, 428 insertions(+), 168 deletions(-) create mode 100644 app/common/AttachmentColumns.ts diff --git a/app/client/components/WidgetFrame.ts b/app/client/components/WidgetFrame.ts index 7722e56b..2a2ba799 100644 --- a/app/client/components/WidgetFrame.ts +++ b/app/client/components/WidgetFrame.ts @@ -306,9 +306,9 @@ export class GristDocAPIImpl implements GristDocAPI { public async listTables(): Promise { // 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) { diff --git a/app/common/ActiveDocAPI.ts b/app/common/ActiveDocAPI.ts index 73af4c22..94ad3cd6 100644 --- a/app/common/ActiveDocAPI.ts +++ b/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; + fetchTable(tableId: string): Promise; /** * Fetches the generated Python code for this document. (TODO rename this misnomer.) diff --git a/app/common/AttachmentColumns.ts b/app/common/AttachmentColumns.ts new file mode 100644 index 00000000..79214b83 --- /dev/null +++ b/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>; + +/** + * 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> = 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 { + const tableId = getTableId(action); + const attColumns = attachmentColumns.get(tableId); + const colIds = getColIdsFromDocAction(action) || []; + const attIds = new Set(); + 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; +} diff --git a/app/common/DocActions.ts b/app/common/DocActions.ts index e9323103..f07e45f2 100644 --- a/app/common/DocActions.ts +++ b/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[]): 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]; + } +} diff --git a/app/common/DocData.ts b/app/common/DocData.ts index 123efa8a..9b185022 100644 --- a/app/common/DocData.ts +++ b/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; +type FetchTableFunc = (tableId: string) => Promise; export class DocData extends ActionDispatcher { private _tables: Map = new Map(); + private _fetchTableFunc: (tableId: string) => Promise; + /** * 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) { diff --git a/app/common/TableData.ts b/app/common/TableData.ts index d4b9c614..e70aada6 100644 --- a/app/common/TableData.ts +++ b/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++; } } } diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 5c43f00e..b179a880 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/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 { + waitForFormulas: boolean = false): Promise { 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 { + waitForFormulas: boolean = false): Promise { 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. diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index 9dca4b37..152a05e2 100644 --- a/app/server/lib/DocApi.ts +++ b/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'); diff --git a/app/server/lib/Export.ts b/app/server/lib/Export.ts index 6876d6d7..26749002 100644 --- a/app/server/lib/Export.ts +++ b/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); diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index f7323349..68f14bdf 100644 --- a/app/server/lib/GranularAccess.ts +++ b/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 { 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> { - const tablesTable = metaDocData.getMetaTable('_grist_Tables'); - const columnsTable = metaDocData.getMetaTable('_grist_Tables_column'); - const attachmentColumns: Map> = 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 { + private async _filterOutgoingDocAction(cursor: ActionCursor): Promise { 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 { + 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(); + 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> { + const empty = new Set(); 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(); - 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>; // 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. } /** diff --git a/app/server/lib/RowAccess.ts b/app/server/lib/RowAccess.ts index 43131ac9..e9965d06 100644 --- a/app/server/lib/RowAccess.ts +++ b/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]; - } -} diff --git a/app/server/lib/Triggers.ts b/app/server/lib/Triggers.ts index e20114ab..e57736b1 100644 --- a/app/server/lib/Triggers.ts +++ b/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}); } } diff --git a/sandbox/gvisor/run.py b/sandbox/gvisor/run.py index b84f3f47..1cf3c238 100755 --- a/sandbox/gvisor/run.py +++ b/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'] = [ diff --git a/test/server/gristClient.ts b/test/server/gristClient.ts index 93538054..b24c7af6 100644 --- a/test/server/gristClient.ts +++ b/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 = []; + 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 { 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 { while (true) { // eslint-disable-line no-constant-condition