diff --git a/app/server/lib/DocClients.ts b/app/server/lib/DocClients.ts index 943debaa..51adf9fa 100644 --- a/app/server/lib/DocClients.ts +++ b/app/server/lib/DocClients.ts @@ -11,6 +11,11 @@ import {sendDocMessage} from 'app/server/lib/Comm'; import {DocSession, OptDocSession} from 'app/server/lib/DocSession'; import * as log from 'app/server/lib/log'; +// Allow tests to impose a serial order for broadcasts if they need that for repeatability. +export const Deps = { + BROADCAST_ORDER: 'parallel' as 'parallel' | 'series', +}; + export class DocClients { private _docSessions: DocSession[] = []; @@ -78,48 +83,62 @@ export class DocClients { public async broadcastDocMessage(client: Client|null, type: string, messageData: any, filterMessage?: (docSession: OptDocSession, messageData: any) => Promise): Promise { - await Promise.all(this._docSessions.map(async curr => { - const fromSelf = (curr.client === client); - try { - // Make sure user still has view access. - await curr.authorizer.assertAccess('viewers'); - if (!filterMessage) { - sendDocMessage(curr.client, curr.fd, type, messageData, fromSelf); - } else { - try { - const filteredMessageData = await filterMessage(curr, messageData); - if (filteredMessageData) { - sendDocMessage(curr.client, curr.fd, type, filteredMessageData, fromSelf); - } else { - this.activeDoc.logDebug(curr, 'skip broadcastDocMessage because it is not allowed for this client'); - } - } catch (e) { - if (e.code && e.code === 'NEED_RELOAD') { - sendDocMessage(curr.client, curr.fd, 'docShutdown', null, fromSelf); - } else { - sendDocMessage(curr.client, curr.fd, 'docUserAction', {error: String(e)}, fromSelf); - } - } - } - } catch (e) { - if (e.code === 'AUTH_NO_VIEW') { - // Skip sending data to this user, they have no view access. - log.rawDebug('skip broadcastDocMessage because AUTH_NO_VIEW', { - docId: curr.authorizer.getDocId(), - ...curr.client.getLogMeta() - }); - // Go further and trigger a shutdown for this user, in case they are granted - // access again later. - sendDocMessage(curr.client, curr.fd, 'docShutdown', null, fromSelf); - } else { - throw(e); - } + const send = (curr: DocSession) => this._send(curr, client, type, messageData, filterMessage); + if (Deps.BROADCAST_ORDER === 'parallel') { + await Promise.all(this._docSessions.map(send)); + } else { + for (const session of this._docSessions) { + await send(session); } - })); + } if (type === "docUserAction" && messageData.docActions) { for (const action of messageData.docActions) { this.activeDoc.docPluginManager.receiveAction(action); } } } + + /** + * Send a message to a single client. See broadcastDocMessage for parameters. + */ + private async _send(target: DocSession, client: Client|null, type: string, messageData: any, + filterMessage?: (docSession: OptDocSession, + messageData: any) => Promise): Promise { + const fromSelf = (target.client === client); + try { + // Make sure user still has view access. + await target.authorizer.assertAccess('viewers'); + if (!filterMessage) { + sendDocMessage(target.client, target.fd, type, messageData, fromSelf); + } else { + try { + const filteredMessageData = await filterMessage(target, messageData); + if (filteredMessageData) { + sendDocMessage(target.client, target.fd, type, filteredMessageData, fromSelf); + } else { + this.activeDoc.logDebug(target, 'skip broadcastDocMessage because it is not allowed for this client'); + } + } catch (e) { + if (e.code && e.code === 'NEED_RELOAD') { + sendDocMessage(target.client, target.fd, 'docShutdown', null, fromSelf); + } else { + sendDocMessage(target.client, target.fd, 'docUserAction', {error: String(e)}, fromSelf); + } + } + } + } catch (e) { + if (e.code === 'AUTH_NO_VIEW') { + // Skip sending data to this user, they have no view access. + log.rawDebug('skip broadcastDocMessage because AUTH_NO_VIEW', { + docId: target.authorizer.getDocId(), + ...target.client.getLogMeta() + }); + // Go further and trigger a shutdown for this user, in case they are granted + // access again later. + sendDocMessage(target.client, target.fd, 'docShutdown', null, fromSelf); + } else { + throw(e); + } + } + } } diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index e5e18816..440cb2ce 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -676,7 +676,7 @@ export class GranularAccess implements GranularAccessForBundle { const tableId = getTableId(data); if (this.getReadPermission(permInfo.getTableAccess(tableId)) === 'mixed') { const readAccessCheck = this._readAccessCheck(docSession); - await this._filterRowsAndCells(cursor, data, data, readAccessCheck, true); + await this._filterRowsAndCells(cursor, data, data, readAccessCheck, {allowRowRemoval: true}); } // Filter columns, omitting any to which the user has no access, regardless of rows. @@ -932,10 +932,14 @@ export class GranularAccess implements GranularAccessForBundle { // Return the results, also applying any cell-level access control. const readAccessCheck = this._readAccessCheck(cursor.docSession); + const filteredDocActions: DocAction[] = []; for (const a of revisedDocActions) { - await this._filterRowsAndCells({...cursor, action: a}, rowsAfter, rowsAfter, readAccessCheck, false); + const {filteredAction} = + await this._filterRowsAndCells({...cursor, action: a}, rowsAfter, rowsAfter, readAccessCheck, + {allowRowRemoval: false, copyOnModify: true}); + if (filteredAction) { filteredDocActions.push(filteredAction); } } - return revisedDocActions; + return filteredDocActions; } /** @@ -947,7 +951,10 @@ export class GranularAccess implements GranularAccessForBundle { // This check applies to data changes only. if (!isDataAction(action)) { return; } const {rowsBefore, rowsAfter} = await this._getRowsForRecAndNewRec(cursor); - await this._filterRowsAndCells(cursor, rowsBefore, rowsAfter, accessCheck, false); + // If any change is needed, this call will fail immediately because we are using + // access checks that throw. + await this._filterRowsAndCells(cursor, rowsBefore, rowsAfter, accessCheck, + {allowRowRemoval: false}); } private async _getRowsBeforeAndAfter(cursor: ActionCursor) { @@ -988,10 +995,15 @@ export class GranularAccess implements GranularAccessForBundle { } /** - * Modify action in place, scrubbing any rows and cells to which access is not granted. - * Returns filteredAction, which is the provided action or null - it is null if the - * action was entirely eliminated (and was not a bulk action). Also returns - * censoredRows, a set of indexes of rows that have a censored value in them. + * Scrub any rows and cells to which access is not granted from an + * action. Returns filteredAction, which is the provided action, a + * modified copy of the provided action, or null. It is null if the + * action was entirely eliminated (and was not a bulk action). It is + * a modified copy if any scrubbing was needed and copyOnModify is + * set, otherwise the original is modified in place. + * + * Also returns censoredRows, a set of indexes of rows that have a + * censored value in them. * * If allowRowRemoval is false, then rows will not be removed, and if the user * does not have access to a row and the action itself is not a remove action, then @@ -1001,17 +1013,20 @@ export class GranularAccess implements GranularAccessForBundle { */ private async _filterRowsAndCells(cursor: ActionCursor, rowsBefore: TableDataAction, rowsAfter: TableDataAction, accessCheck: IAccessCheck, - allowRowRemoval: boolean): Promise<{ + options: { + allowRowRemoval?: boolean, + copyOnModify?: boolean, + }): Promise<{ filteredAction: DocAction | null, censoredRows: Set }> { const censoredRows = new Set(); const ruler = await this._getRuler(cursor); const {docSession, action} = cursor; - let filteredAction: DocAction | null = action; if (action && isSchemaAction(action)) { - return {filteredAction, censoredRows}; + return {filteredAction: action, censoredRows}; } + let filteredAction: DocAction | null = action; // For user convenience, for creations and deletions we equate rec and newRec. // This makes writing rules that control multiple permissions easier to write in @@ -1029,16 +1044,25 @@ export class GranularAccess implements GranularAccessForBundle { const input: AclMatchInput = {user: await this._getUser(docSession), rec, newRec}; const [, tableId, , colValues] = action; + let filteredColValues: ColValues | BulkColValues | undefined | null = null; const rowIds = getRowIdsFromDocAction(action); const toRemove: number[] = []; + // Call this to make sure we are modifying a copy, not the original, if copyOnModify is set. + const copyOnNeed = () => { + if (filteredColValues === null) { + filteredAction = options?.copyOnModify ? cloneDeep(action) : action; + filteredColValues = filteredAction[3]; + } + return filteredColValues; + }; let censorAt: (colId: string, idx: number) => void; if (colValues === undefined) { censorAt = () => 1; } else if (Array.isArray(action[2])) { - censorAt = (colId, idx) => (colValues as BulkColValues)[colId][idx] = ['C']; // censored + censorAt = (colId, idx) => (copyOnNeed() as BulkColValues)[colId][idx] = ['C']; // censored } else { - censorAt = (colId) => (colValues as ColValues)[colId] = ['C']; // censored + censorAt = (colId) => (copyOnNeed() as ColValues)[colId] = ['C']; // censored } // These map an index of a row in the action to its index in rowsBefore and in rowsAfter. @@ -1076,15 +1100,16 @@ export class GranularAccess implements GranularAccessForBundle { } if (toRemove.length > 0) { - if (allowRowRemoval) { - if (Array.isArray(action[2])) { - this._removeRowsAt(toRemove, action[2], action[3]); + if (options.allowRowRemoval) { + copyOnNeed(); + if (Array.isArray(filteredAction[2])) { + this._removeRowsAt(toRemove, filteredAction[2], filteredAction[3]); } else { filteredAction = null; } } else { // Artificially introduced removals are ok, otherwise this is suspect. - if (action[0] !== 'RemoveRecord' && action[0] !== 'BulkRemoveRecord') { + if (filteredAction[0] !== 'RemoveRecord' && filteredAction[0] !== 'BulkRemoveRecord') { throw new Error('Unexpected row removal'); } } @@ -1499,7 +1524,7 @@ export class GranularAccess implements GranularAccessForBundle { const {rowsBefore, rowsAfter} = await this._getRowsForRecAndNewRec(cursor); const {censoredRows, filteredAction} = await this._filterRowsAndCells({...cursor, action: cloneDeep(action)}, rowsBefore, rowsAfter, accessCheck, - true); + {allowRowRemoval: true}); if (filteredAction === null) { return []; }