From ea71312d0e5d062a3a8c84be46a77f7774ea737b Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Tue, 15 Nov 2022 09:37:48 -0500 Subject: [PATCH] (core) deal with write access for attachments Summary: Attachments are a special case for granular access control. A user is now allowed to read a given attachment if they have read access to a cell containing its id. So when a user writes to a cell in an attachment column, it is important that they can only write the ids of cells to which they have access. This diff allows a user to add an attachment id in a cell if: * The user already has access to that a attachment via some existing cell, or * The user recently updated the attachment, or * The attachment change is from an undo/redo of a previous action attributed to that user Test Plan: Updated tests Reviewers: georgegevoian, dsagal Reviewed By: georgegevoian, dsagal Differential Revision: https://phab.getgrist.com/D3681 --- app/client/widgets/AttachmentsEditor.ts | 1 + app/common/ActionBundle.ts | 2 + app/common/ActiveDocAPI.ts | 10 +- app/common/GranularAccessClause.ts | 1 + app/server/lib/ActiveDoc.ts | 138 ++++++++------ app/server/lib/DocApi.ts | 2 +- app/server/lib/DocStorage.ts | 4 + app/server/lib/DocWorker.ts | 4 +- app/server/lib/GranularAccess.ts | 227 ++++++++++++++++++++++-- app/server/lib/RowAccess.ts | 25 ++- app/server/lib/Sharing.ts | 16 +- 11 files changed, 344 insertions(+), 86 deletions(-) diff --git a/app/client/widgets/AttachmentsEditor.ts b/app/client/widgets/AttachmentsEditor.ts index badb2ba6..2c8c5461 100644 --- a/app/client/widgets/AttachmentsEditor.ts +++ b/app/client/widgets/AttachmentsEditor.ts @@ -203,6 +203,7 @@ export class AttachmentsEditor extends NewBaseEditor { ...this._docComm.getUrlParams(), name: filename, ...cell, + maybeNew: 1, // The attachment may be uploaded by the user but not stored in the cell yet. attId, ...(inline ? {inline: 1} : {}) }); diff --git a/app/common/ActionBundle.ts b/app/common/ActionBundle.ts index 336b8aa7..f48e584a 100644 --- a/app/common/ActionBundle.ts +++ b/app/common/ActionBundle.ts @@ -3,6 +3,7 @@ * See also EncActionBundle for how these are packaged for encryption. */ +import {ApplyUAOptions} from 'app/common/ActiveDocAPI'; import {DocAction, UserAction} from 'app/common/DocActions'; import {RowCounts} from 'app/common/DocUsage'; @@ -50,6 +51,7 @@ export function getEnvContent(items: Array>): Conte export interface UserActionBundle { info: ActionInfo; userActions: UserAction[]; + options?: ApplyUAOptions; } // ActionBundle as received from the sandbox. It does not have some action metadata, but does have diff --git a/app/common/ActiveDocAPI.ts b/app/common/ActiveDocAPI.ts index 87a86194..181d1283 100644 --- a/app/common/ActiveDocAPI.ts +++ b/app/common/ActiveDocAPI.ts @@ -12,10 +12,18 @@ export interface ApplyUAOptions { desc?: string; // Overrides the description of the action. otherId?: number; // For undo/redo; the actionNum of the original action to which it applies. linkId?: number; // For bundled actions, actionNum of the previous action in the bundle. - bestEffort?: boolean; // If set, action may be applied in part if it cannot be applied completely. parseStrings?: boolean; // If true, parses string values in some actions based on the column } +export interface ApplyUAExtendedOptions extends ApplyUAOptions { + bestEffort?: boolean; // If set, action may be applied in part if it cannot be applied completely. + fromOwnHistory?: boolean; // If set, action is confirmed to be a redo/undo taken from history, from + // an action marked as being by the current user. + oldestSource?: number; // If set, gives the timestamp of the oldest source the undo/redo + // action was built from, expressed as number of milliseconds + // elapsed since January 1, 1970 00:00:00 UTC +} + export interface ApplyUAResult { actionNum: number; // number of the action that got recorded. retValues: any[]; // array of return values, one for each of the passed-in user actions. diff --git a/app/common/GranularAccessClause.ts b/app/common/GranularAccessClause.ts index cff5b277..402e843e 100644 --- a/app/common/GranularAccessClause.ts +++ b/app/common/GranularAccessClause.ts @@ -46,6 +46,7 @@ export interface UserInfo { LinkKey: Record; UserID: number | null; UserRef: string | null; + SessionID: string | null; [attributes: string]: unknown; toJSON(): {[key: string]: any}; } diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 390aeb1b..fa172a83 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -15,6 +15,7 @@ import {ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup'; import {ActionSummary} from "app/common/ActionSummary"; import { AclTableDescription, + ApplyUAExtendedOptions, ApplyUAOptions, ApplyUAResult, DataSourceTransformed, @@ -55,7 +56,6 @@ import { RowCounts, } from 'app/common/DocUsage'; import {normalizeEmail} from 'app/common/emails'; -import {ErrorWithCode} from 'app/common/ErrorWithCode'; import {Product} from 'app/common/Features'; import {FormulaProperties, getFormulaProperties} from 'app/common/GranularAccessClause'; import {parseUrlId} from 'app/common/gristUrls'; @@ -113,7 +113,7 @@ import { makeExceptionalDocSession, OptDocSession } from './DocSession'; -import {createAttachmentsIndex, DocStorage} from './DocStorage'; +import {createAttachmentsIndex, DocStorage, REMOVE_UNUSED_ATTACHMENTS_DELAY} from './DocStorage'; import {expandQuery} from './ExpandedQuery'; import {GranularAccess, GranularAccessForBundle} from './GranularAccess'; import {OnDemandActions} from './OnDemandActions'; @@ -121,6 +121,7 @@ import {getLogMetaFromDocSession, timeoutReached} from './serverUtils'; import {findOrAddAllEnvelope, Sharing} from './Sharing'; import cloneDeep = require('lodash/cloneDeep'); import flatten = require('lodash/flatten'); +import pick = require('lodash/pick'); import remove = require('lodash/remove'); import sum = require('lodash/sum'); import without = require('lodash/without'); @@ -141,9 +142,6 @@ const ACTIVEDOC_TIMEOUT = (process.env.NODE_ENV === 'production') ? 30 : 5; // We'll wait this long between re-measuring sandbox memory. const MEMORY_MEASUREMENT_INTERVAL_MS = 60 * 1000; -// Cleanup expired attachments every hour (also happens when shutting down) -const REMOVE_UNUSED_ATTACHMENTS_DELAY = {delayMs: 60 * 60 * 1000, varianceMs: 30 * 1000}; - // Apply the UpdateCurrentTime user action every hour const UPDATE_CURRENT_TIME_DELAY = {delayMs: 60 * 60 * 1000, varianceMs: 30 * 1000}; @@ -571,6 +569,12 @@ export class ActiveDoc extends EventEmitter { } finally { this._docManager.removeActiveDoc(this); } + try { + await this._granularAccess.close(); + } catch (err) { + // This should not happen. + this._log.error(docSession, "failed to shutdown granular access", err); + } this._log.debug(docSession, "shutdown complete"); } @@ -844,6 +848,7 @@ export class ActiveDoc extends EventEmitter { this._updateAttachmentsSize().catch(e => { this._log.warn(docSession, 'failed to update attachments size', e); }); + await this._granularAccess.noteUploads(docSession, result.retValues); return result.retValues; } finally { await globalUploadSet.cleanup(uploadId); @@ -868,47 +873,40 @@ export class ActiveDoc extends EventEmitter { /** * Given a _gristAttachments record, returns a promise for the attachment data. + * Can optionally take a cell in which the attachment is expected to be + * referenced, and/or a `maybeNew` flag which, when set, specifies that the + * attachment may be a recent upload that is not yet referenced in the document. * @returns {Promise} Promise for the data of this attachment; rejected on error. */ public async getAttachmentData(docSession: OptDocSession, attRecord: MetaRowRecord<"_grist_Attachments">, - cell?: SingleCell): Promise { + options?: { + cell?: SingleCell, + maybeNew?: boolean, + }): Promise { const attId = attRecord.id; const fileIdent = attRecord.fileIdent; + const cell = options?.cell; + const maybeNew = options?.maybeNew; if ( await this._granularAccess.canReadEverything(docSession) || await this.canDownload(docSession) ) { // Do not need to sweat over access to attachments if user can // read everything or download everything. - } else if (cell) { - // Only provide the download if the user has access to the cell - // they specified, and that cell is in an attachment column, - // and the cell contains the specified attachment. - await this._granularAccess.assertAttachmentAccess(docSession, cell, attId); } else { - // Find cells that refer to the given attachment. - const cells = await this.docStorage.findAttachmentReferences(attId); - // Run through them to see if the user has access to any of them. - // If so, we'll allow the download. We'd expect in a typical document - // this this will be a small list of cells, typically 1 or less, but - // of course extreme cases are possible. - let goodCell: SingleCell|undefined; - for (const possibleCell of cells) { - try { - await this._granularAccess.assertAttachmentAccess(docSession, possibleCell, attId); - goodCell = possibleCell; - break; - } catch (e) { - if (e instanceof ErrorWithCode && e.code === 'ACL_DENY') { - continue; - } - throw e; + if (maybeNew && await this._granularAccess.isAttachmentUploadedByUser(docSession, attId)) { + // Fine, this is an attachment the user uploaded (recently). + } else if (cell) { + // Only provide the download if the user has access to the cell + // they specified, and that cell is in an attachment column, + // and the cell contains the specified attachment. + await this._granularAccess.assertAttachmentAccess(docSession, cell, attId); + } else { + if (!await this._granularAccess.findAttachmentCellForUser(docSession, attId)) { + // We found no reason to allow this user to access the attachment. + throw new ApiError('Cannot access attachment', 403); } } - if (!goodCell) { - // We found no reason to allow this user to access the attachment. - throw new ApiError('Cannot access attachment', 403); - } } const data = await this.docStorage.getFileData(fileIdent); if (!data) { throw new ApiError("Invalid attachment identifier", 404); } @@ -1165,24 +1163,9 @@ export class ActiveDoc extends EventEmitter { * actionGroup. */ public async applyUserActions(docSession: OptDocSession, actions: UserAction[], - options?: ApplyUAOptions): Promise { - assert(Array.isArray(actions), "`actions` parameter should be an array."); - // Be careful not to sneak into user action queue before Calculate action, otherwise - // there'll be a deadlock. - await this.waitForInitialization(); - - if ( - this.dataLimitStatus === "deleteOnly" && - !actions.every(action => [ - 'RemoveTable', 'RemoveColumn', 'RemoveRecord', 'BulkRemoveRecord', - 'RemoveViewSection', 'RemoveView', 'ApplyUndoActions', 'RespondToRequests', - ].includes(action[0] as string)) - ) { - throw new Error("Document is in delete-only mode"); - } - - // Granular access control implemented in _applyUserActions. - return await this._applyUserActions(docSession, actions, options); + unsanitizedOptions?: ApplyUAOptions): Promise { + const options = sanitizeApplyUAOptions(unsanitizedOptions); + return this._applyUserActionsWithExtendedOptions(docSession, actions, options); } /** @@ -1200,12 +1183,25 @@ export class ActiveDoc extends EventEmitter { actionNums: number[], actionHashes: string[], undo: boolean, - options?: ApplyUAOptions): Promise { + unsanitizedOptions?: ApplyUAOptions): Promise { + const options = sanitizeApplyUAOptions(unsanitizedOptions); const actionBundles = await this._actionHistory.getActions(actionNums); + let fromOwnHistory: boolean = true; + const user = getDocSessionUser(docSession); + let oldestSource: number = Date.now(); for (const [index, bundle] of actionBundles.entries()) { const actionNum = actionNums[index]; const actionHash = actionHashes[index]; if (!bundle) { throw new Error(`Could not find actionNum ${actionNum}`); } + const info = bundle.info[1]; + const bundleEmail = info.user || ''; + const sessionEmail = user?.email || ''; + if (normalizeEmail(sessionEmail) !== normalizeEmail(bundleEmail)) { + fromOwnHistory = false; + } + if (info.time && info.time < oldestSource) { + oldestSource = info.time; + } if (actionHash !== bundle.actionHash) { throw new Error(`Hash mismatch for actionNum ${actionNum}: ` + `expected ${actionHash} but got ${bundle.actionHash}`); @@ -1221,7 +1217,10 @@ export class ActiveDoc extends EventEmitter { // It could be that error cases and timing etc leak some info prior to this // point. // Undos are best effort now by default. - return this.applyUserActions(docSession, actions, {bestEffort: undo, ...(options||{})}); + return this._applyUserActionsWithExtendedOptions( + docSession, actions, {bestEffort: undo, + oldestSource, + fromOwnHistory, ...(options||{})}); } /** @@ -1685,8 +1684,9 @@ export class ActiveDoc extends EventEmitter { * granular access rules. */ public getGranularAccessForBundle(docSession: OptDocSession, docActions: DocAction[], undo: DocAction[], - userActions: UserAction[], isDirect: boolean[]): GranularAccessForBundle { - this._granularAccess.getGranularAccessForBundle(docSession, docActions, undo, userActions, isDirect); + userActions: UserAction[], isDirect: boolean[], + options: ApplyUAOptions|null): GranularAccessForBundle { + this._granularAccess.getGranularAccessForBundle(docSession, docActions, undo, userActions, isDirect, options); return this._granularAccess; } @@ -1784,7 +1784,7 @@ export class ActiveDoc extends EventEmitter { */ @ActiveDoc.keepDocOpen protected async _applyUserActions(docSession: OptDocSession, actions: UserAction[], - options: ApplyUAOptions = {}): Promise { + options: ApplyUAExtendedOptions = {}): Promise { const client = docSession.client; this._log.debug(docSession, "_applyUserActions(%s, %s)%s", client, shortDesc(actions), @@ -1796,13 +1796,14 @@ export class ActiveDoc extends EventEmitter { } if (options?.bestEffort) { - actions = await this._granularAccess.prefilterUserActions(docSession, actions); + actions = await this._granularAccess.prefilterUserActions(docSession, actions, options); } await this._granularAccess.checkUserActions(docSession, actions); // Create the UserActionBundle. const action: UserActionBundle = { info: this._makeInfo(docSession, options), + options, userActions: actions, }; @@ -1818,6 +1819,27 @@ export class ActiveDoc extends EventEmitter { return result; } + private async _applyUserActionsWithExtendedOptions(docSession: OptDocSession, actions: UserAction[], + options?: ApplyUAExtendedOptions): Promise { + assert(Array.isArray(actions), "`actions` parameter should be an array."); + // Be careful not to sneak into user action queue before Calculate action, otherwise + // there'll be a deadlock. + await this.waitForInitialization(); + + if ( + this.dataLimitStatus === "deleteOnly" && + !actions.every(action => [ + 'RemoveTable', 'RemoveColumn', 'RemoveRecord', 'BulkRemoveRecord', + 'RemoveViewSection', 'RemoveView', 'ApplyUndoActions', 'RespondToRequests', + ].includes(action[0] as string)) + ) { + throw new Error("Document is in delete-only mode"); + } + + // Granular access control implemented in _applyUserActions. + return await this._applyUserActions(docSession, actions, options); + } + /** * Create a new document file without using or initializing the data engine. */ @@ -2306,3 +2328,7 @@ export function tableIdToRef(metaTables: { [p: string]: TableDataAction }, table } return tableRefs[tableRowIndex]; } + +export function sanitizeApplyUAOptions(options?: ApplyUAOptions): ApplyUAOptions { + return pick(options||{}, ['desc', 'otherId', 'linkId', 'parseStrings']); +} diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index 966838a0..6ba3bbd3 100644 --- a/app/server/lib/DocApi.ts +++ b/app/server/lib/DocApi.ts @@ -290,7 +290,7 @@ export class DocWorkerApi { const ext = path.extname(fileIdent); const origName = attRecord.fileName as string; const fileName = ext ? path.basename(origName, path.extname(origName)) + ext : origName; - const fileData = await activeDoc.getAttachmentData(docSessionFromRequest(req), attRecord, cell); + const fileData = await activeDoc.getAttachmentData(docSessionFromRequest(req), attRecord, {cell}); res.status(200) .type(ext) // Construct a content-disposition header of the form 'attachment; filename="NAME"' diff --git a/app/server/lib/DocStorage.ts b/app/server/lib/DocStorage.ts index 844c40ca..6a07d0d3 100644 --- a/app/server/lib/DocStorage.ts +++ b/app/server/lib/DocStorage.ts @@ -47,6 +47,10 @@ const PENDING_VALUE = [GristObjCode.Pending]; // that someone would delete a reference to an attachment and then undo that action this many days later. export const ATTACHMENTS_EXPIRY_DAYS = 7; +// Cleanup expired attachments every hour (also happens when shutting down). +export const REMOVE_UNUSED_ATTACHMENTS_DELAY = {delayMs: 60 * 60 * 1000, varianceMs: 30 * 1000}; + + export class DocStorage implements ISQLiteDB, OnDemandStorage { // ====================================================================== diff --git a/app/server/lib/DocWorker.ts b/app/server/lib/DocWorker.ts index 880c2731..180fa2e5 100644 --- a/app/server/lib/DocWorker.ts +++ b/app/server/lib/DocWorker.ts @@ -2,6 +2,7 @@ * DocWorker collects the methods and endpoints that relate to a single Grist document. * In hosted environment, this comprises the functionality of the DocWorker instance type. */ +import {isAffirmative} from 'app/common/gutil'; import {HomeDBManager} from 'app/gen-server/lib/HomeDBManager'; import {ActionHistoryImpl} from 'app/server/lib/ActionHistoryImpl'; import {assertAccess, getOrSetDocAuth, RequestWithLogin} from 'app/server/lib/Authorizer'; @@ -42,6 +43,7 @@ export class DocWorker { const tableId = stringParam(req.query.tableId, 'tableId'); const rowId = integerParam(req.query.rowId, 'rowId'); const cell = {colId, tableId, rowId}; + const maybeNew = isAffirmative(req.query.maybeNew); const attId = integerParam(req.query.attId, 'attId'); const attRecord = activeDoc.getAttachmentMetadata(attId); const ext = path.extname(attRecord.fileIdent); @@ -54,7 +56,7 @@ export class DocWorker { // Construct a content-disposition header of the form 'inline|attachment; filename="NAME"' const contentDispType = inline ? "inline" : "attachment"; const contentDispHeader = contentDisposition(stringParam(req.query.name, 'name'), {type: contentDispType}); - const data = await activeDoc.getAttachmentData(docSession, attRecord, cell); + const data = await activeDoc.getAttachmentData(docSession, attRecord, {cell, maybeNew}); res.status(200) .type(ext) .set('Content-Disposition', contentDispHeader) diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 5a303a6d..700e12bb 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -2,8 +2,9 @@ import { ALL_PERMISSION_PROPS } from 'app/common/ACLPermissions'; import { ACLRuleCollection, SPECIAL_RULES_TABLE_ID } from 'app/common/ACLRuleCollection'; import { ActionGroup } from 'app/common/ActionGroup'; import { createEmptyActionSummary } from 'app/common/ActionSummary'; -import { ServerQuery } from 'app/common/ActiveDocAPI'; +import { ApplyUAExtendedOptions, ServerQuery } from 'app/common/ActiveDocAPI'; import { ApiError } from 'app/common/ApiError'; +import { MapWithTTL } from 'app/common/AsyncCreate'; import { AddRecord, BulkAddRecord, @@ -27,7 +28,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, pruneArray } from 'app/common/gutil'; +import { getSetMapValue, isNonNullish, isNumber, 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'; @@ -37,12 +38,13 @@ import { compileAclFormula } from 'app/server/lib/ACLFormula'; import { DocClients } from 'app/server/lib/DocClients'; import { getDocSessionAccess, getDocSessionAltSessionId, getDocSessionUser, OptDocSession } from 'app/server/lib/DocSession'; -import { DocStorage } from 'app/server/lib/DocStorage'; +import { DocStorage, REMOVE_UNUSED_ATTACHMENTS_DELAY } from 'app/server/lib/DocStorage'; import log from 'app/server/lib/log'; import { IPermissionInfo, PermissionInfo, PermissionSetWithContext } from 'app/server/lib/PermissionInfo'; import { TablePermissionSetWithContext } from 'app/server/lib/PermissionInfo'; import { integerParam } from 'app/server/lib/requestUtils'; -import { getRelatedRows, getRowIdsFromDocAction } from 'app/server/lib/RowAccess'; +import { getColIdsFromDocAction, getColValuesFromDocAction, getRelatedRows, + getRowIdsFromDocAction } from 'app/server/lib/RowAccess'; import cloneDeep = require('lodash/cloneDeep'); import fromPairs = require('lodash/fromPairs'); import memoize = require('lodash/memoize'); @@ -176,6 +178,19 @@ const OTHER_RECOGNIZED_ACTIONS = new Set([ 'RemoveViewSection', ]); +// When an attachment is uploaded, it isn't immediately added to a cell in +// the document. We grant the uploader a special period where they can freely +// add or re-add the attachment to the document without access control fuss. +// We keep that period within the time range where an unused attachment +// would get deleted. +const UPLOADED_ATTACHMENT_OWNERSHIP_PERIOD = + (REMOVE_UNUSED_ATTACHMENTS_DELAY.delayMs - REMOVE_UNUSED_ATTACHMENTS_DELAY.varianceMs) / 2; + +// When a user undoes their own action or actions, checks of attachment ownership +// are handled specially. This special handling will not apply for undoes of actions +// older than this limit. +const HISTORICAL_ATTACHMENT_OWNERSHIP_PERIOD = 24 * 60 * 60 * 1000; + interface DocUpdateMessage { actionGroup: ActionGroup; docActions: DocAction[]; @@ -229,6 +244,7 @@ export class GranularAccess implements GranularAccessForBundle { // garbage-collection once docSession is no longer in use. private _userAttributesMap = new WeakMap(); private _prevUserAttributesMap: WeakMap|undefined; + private _attachmentUploads = new MapWithTTL(UPLOADED_ATTACHMENT_OWNERSHIP_PERIOD); // When broadcasting a sequence of DocAction[]s, this contains the state of // affected rows for the relevant table before and after each DocAction. It @@ -251,6 +267,7 @@ export class GranularAccess implements GranularAccessForBundle { // Flag for whether doc actions mention a rule change, even if passive due to // schema changes. hasAnyRuleChange: boolean, + options: ApplyUAExtendedOptions|null, }|null; public constructor( @@ -263,8 +280,13 @@ export class GranularAccess implements GranularAccessForBundle { private _docId: string) { } + public async close() { + this._attachmentUploads.clear(); + } + public getGranularAccessForBundle(docSession: OptDocSession, docActions: DocAction[], undo: DocAction[], - userActions: UserAction[], isDirect: boolean[]): void { + userActions: UserAction[], isDirect: boolean[], + options: ApplyUAExtendedOptions|null): void { if (this._activeBundle) { throw new Error('Cannot start a bundle while one is already in progress'); } // This should never happen - attempts to write to a pre-fork session should be // caught by an Authorizer. But let's be paranoid, since we may be pretending to @@ -273,7 +295,8 @@ export class GranularAccess implements GranularAccessForBundle { if (docSession.forkingAsOwner) { throw new Error('Should never modify a prefork'); } this._activeBundle = { docSession, docActions, undo, userActions, isDirect, - applied: false, hasDeliberateRuleChange: false, hasAnyRuleChange: false + applied: false, hasDeliberateRuleChange: false, hasAnyRuleChange: false, + options, }; this._activeBundle.hasDeliberateRuleChange = scanActionsRecursively(userActions, (a) => isAclTable(String(a[1]))); @@ -300,6 +323,16 @@ export class GranularAccess implements GranularAccessForBundle { return access.getUser(); } + /** + * Represent fields from the session in an input object for ACL rules. + * Just one field currently, "user". + */ + public async inputs(docSession: OptDocSession): Promise { + return { + user: await this._getUser(docSession), + }; + } + /** * Check whether user has any access to table. */ @@ -348,7 +381,7 @@ export class GranularAccess implements GranularAccessForBundle { return fail(); } const rec = new RecordView(rows, 0); - const input: AclMatchInput = {user: await this._getUser(docSession), rec, newRec: rec}; + const input: AclMatchInput = {...await this.inputs(docSession), rec, newRec: rec}; const rowPermInfo = new PermissionInfo(this._ruler.ruleCollection, input); const rowAccess = rowPermInfo.getTableAccess(cell.tableId).perms.read; if (rowAccess === 'deny') { fail(); } @@ -382,6 +415,42 @@ export class GranularAccess implements GranularAccessForBundle { } } + /** + * Check whether the specified attachment is known to have been uploaded + * by the user (identified by SessionID) recently. + */ + public async isAttachmentUploadedByUser(docSession: OptDocSession, attId: number): Promise { + const user = await this.getUser(docSession); + const id = user.SessionID || ''; + return (this._attachmentUploads.get(attId) === id); + } + + /** + * Find a cell in an attachment column that contains the specified attachment, + * and which is accessible by the user associated with the session. + */ + public async findAttachmentCellForUser(docSession: OptDocSession, attId: number): Promise { + // Find cells that refer to the given attachment. + const cells = await this._docStorage.findAttachmentReferences(attId); + // Run through them to see if the user has access to any of them. + // We'd expect in a typical document that this will be a small + // list of cells, typically 1 or less, but of course extreme cases + // are possible. + for (const possibleCell of cells) { + try { + await this.assertAttachmentAccess(docSession, possibleCell, attId); + return possibleCell; + } catch (e) { + if (e instanceof ErrorWithCode && e.code === 'ACL_DENY') { + continue; + } + throw e; + } + } + // Nothing found. + return undefined; + } + /** * Called after UserAction[]s have been applied in the sandbox, and DocAction[]s have been * computed, but before we have committed those DocAction[]s to the database. If this @@ -612,7 +681,8 @@ export class GranularAccess implements GranularAccessForBundle { * Any filtering done here is NOT a security measure, and the output should * not be granted any level of automatic trust. */ - public async prefilterUserActions(docSession: OptDocSession, actions: UserAction[]): Promise { + public async prefilterUserActions(docSession: OptDocSession, actions: UserAction[], + options: ApplyUAExtendedOptions|null): Promise { // Currently we only attempt prefiltering for an ApplyUndoActions. if (actions.length !== 1) { return actions; } const userAction = actions[0]; @@ -646,7 +716,7 @@ export class GranularAccess implements GranularAccessForBundle { // any case (though we could rearrange to limit how undo actions are // requested). this.getGranularAccessForBundle(docSession, docActions, [], docActions, - docActions.map(() => true)); + docActions.map(() => true), options); for (const [actionIdx, action] of docActions.entries()) { // A single action might contain forbidden material at cell, row, column, // or table level. Retaining permitted material may require refactoring the @@ -880,6 +950,36 @@ export class GranularAccess implements GranularAccessForBundle { (_docSession) => this._filterDocUpdate(_docSession, message)); } + /** + * Called when uploads occur. We record the fact that the specified attachment + * ids originated in uploads by the current user, for a certain length of time. + * During that time, attempts by the user to use these attachment ids in an + * attachment column will be accepted. The user is identified by SessionID, + * which is a user id for logged in users, and a session-unique id for + * anonymous users accessing Grist from a browser. + * + * A remaining weakness of this protection could be if attachment ids were + * reused, and reused quickly. Attachments can be deleted after + * REMOVE_UNUSED_ATTACHMENTS_DELAY and on document shutdown. We keep + * UPLOADED_ATTACHMENT_OWNERSHIP_PERIOD less than REMOVE_UNUSED_ATTACHMENTS_DELAY, + * and wipe our records on document shutdown. + */ + public async noteUploads(docSession: OptDocSession, attIds: number[]) { + const user = await this.getUser(docSession); + const id = user.SessionID; + if (!id) { + log.rawError('noteUploads needs a SessionID', { + docId: this._docId, + attIds, + userId: user.UserID, + }); + return; + } + for (const attId of attIds) { + this._attachmentUploads.set(attId, id); + } + } + // Remove cached access information for a given session. public flushAccess(docSession: OptDocSession) { this._ruler.flushAccess(docSession); @@ -1465,7 +1565,7 @@ export class GranularAccess implements GranularAccessForBundle { const rec = new RecordView(rowsRec, undefined); const newRec = new RecordView(rowsNewRec, undefined); - const input: AclMatchInput = {user: await this._getUser(docSession), rec, newRec}; + const input: AclMatchInput = {...await this.inputs(docSession), rec, newRec}; const [, tableId, , colValues] = action; let filteredColValues: ColValues | BulkColValues | undefined | null = null; @@ -1547,7 +1647,7 @@ export class GranularAccess implements GranularAccessForBundle { colId?: string): Promise { const ruler = await this._getRuler(cursor); const rec = new RecordView(data, undefined); - const input: AclMatchInput = {user: await this._getUser(cursor.docSession), rec}; + const input: AclMatchInput = {...await this.inputs(cursor.docSession), rec}; const [, tableId, rowIds] = data; const toRemove: number[] = []; @@ -1893,7 +1993,9 @@ export class GranularAccess implements GranularAccessForBundle { const needMeta = docActions.some(a => isSchemaAction(a) || getTableId(a).startsWith('_grist_')); if (!needMeta) { // Sometimes, the intermediate states are trivial. - return docActions.map(action => ({action})); + // TODO: look into whether it would be worth caching attachment columns. + const attachmentColumns = this._getAttachmentColumns(this._docData); + return docActions.map(action => ({action, attachmentColumns})); } const metaDocData = new DocData( async (tableId) => { @@ -1944,11 +2046,33 @@ export class GranularAccess implements GranularAccessForBundle { replaceRuler = false; } step.ruler = ruler; + step.attachmentColumns = this._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 @@ -2095,6 +2219,7 @@ export class GranularAccess implements GranularAccessForBundle { } private async _checkIncomingDocAction(cursor: ActionCursor): Promise { + await this._checkIncomingAttachmentChanges(cursor); const {action, docSession} = cursor; const accessCheck = await this._getAccessForActionType(docSession, action, 'fatal'); const tableId = getTableId(action); @@ -2111,6 +2236,69 @@ export class GranularAccess implements GranularAccessForBundle { this._pruneColumns(action, permInfo, tableId, accessCheck); } + /** + * Take a look at the DocAction and see if it might allow the user to + * introduce attachment ids into a cell. If so, make sure the user + * has the right to access any attachments mentioned. + */ + private async _checkIncomingAttachmentChanges(cursor: ActionCursor): Promise { + const options = this._activeBundle?.options; + if (options?.fromOwnHistory && options.oldestSource && + Date.now() - options.oldestSource < HISTORICAL_ATTACHMENT_OWNERSHIP_PERIOD) { + return; + } + const {action, docSession} = cursor; + if (!isDataAction(action)) { return; } + if (isRemoveRecordAction(action)) { return; } + const tableId = getTableId(action); + const step = await this._getMetaStep(cursor); + const attachmentColumns = step.attachmentColumns; + if (!attachmentColumns) { return; } + 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, + }); + } + } + } + private async _getRuler(cursor: ActionCursor) { if (cursor.actionIdx === null) { return this._ruler; } const step = await this._getMetaStep(cursor); @@ -2192,7 +2380,7 @@ export class GranularAccess implements GranularAccessForBundle { }; const ruler = await this._getRuler(cursor); const permInfo = await ruler.getAccess(docSession); - const user = await this._getUser(docSession); + const inputs = await this.inputs(docSession); // Cache some data, as they are checked. const readRows = memoize(this._fetchQueryFromDB.bind(this)); const hasAccess = async (cell: SingleCell) => { @@ -2223,7 +2411,7 @@ export class GranularAccess implements GranularAccessForBundle { } } const rec = rows ? new RecordView(rows, 0) : undefined; - const input: AclMatchInput = {user, rec, newRec: rec}; + const input: AclMatchInput = {...inputs, rec, newRec: rec}; const rowPermInfo = new PermissionInfo(ruler.ruleCollection, input); const rowAccess = rowPermInfo.getTableAccess(cell.tableId).perms.read; if (rowAccess === 'deny') { return false; } @@ -2286,7 +2474,7 @@ export class Ruler { // TODO The intent of caching is to avoid duplicating rule evaluations while processing a // single request. Caching based on docSession is riskier since those persist across requests. return getSetMapValue(this._permissionInfoMap as Map>, docSession, - async () => new PermissionInfo(this.ruleCollection, {user: await this._owner.getUser(docSession)})); + async () => new PermissionInfo(this.ruleCollection, await this._owner.inputs(docSession))); } public flushAccess(docSession: OptDocSession) { @@ -2314,6 +2502,7 @@ export class Ruler { export interface RulerOwner { getUser(docSession: OptDocSession): Promise; + inputs(docSession: OptDocSession): Promise; } /** @@ -2331,6 +2520,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 } /** @@ -2497,7 +2687,7 @@ class CellAccessHelper { private _tableAccess: Map = new Map(); private _rowPermInfo: Map> = new Map(); private _rows: Map = new Map(); - private _user!: UserInfo; + private _inputs!: AclMatchInput; constructor( private _granular: GranularAccess, @@ -2511,7 +2701,7 @@ class CellAccessHelper { * Resolves access for all cells, and save the results in the cache. */ public async calculate(cells: SingleCell[]) { - this._user = await this._granular.getUser(this._docSession); + this._inputs = await this._granular.inputs(this._docSession); const tableIds = new Set(cells.map(cell => cell.tableId)); for (const tableId of tableIds) { this._tableAccess.set(tableId, await this._granular.hasTableAccess(this._docSession, tableId)); @@ -2521,7 +2711,7 @@ class CellAccessHelper { for(const [idx, rowId] of rows[2].entries()) { if (rowIds.has(rowId) === false) { continue; } const rec = new RecordView(rows, idx); - const input: AclMatchInput = {user: this._user, rec, newRec: rec}; + const input: AclMatchInput = {...this._inputs, rec, newRec: rec}; const rowPermInfo = new PermissionInfo(this._ruler.ruleCollection, input); if (!this._rowPermInfo.has(tableId)) { this._rowPermInfo.set(tableId, new Map()); @@ -2892,6 +3082,7 @@ export class User implements UserInfo { public Origin: string | null = null; public LinkKey: Record = {}; public Email: string | null = null; + public SessionID: string | null = null; public UserRef: string | null = null; [attribute: string]: any; diff --git a/app/server/lib/RowAccess.ts b/app/server/lib/RowAccess.ts index 4cef855d..43131ac9 100644 --- a/app/server/lib/RowAccess.ts +++ b/app/server/lib/RowAccess.ts @@ -1,5 +1,6 @@ -import { AddRecord, BulkAddRecord, BulkRemoveRecord, BulkUpdateRecord, DocAction, getTableId, - RemoveRecord, ReplaceTableData, TableDataAction, UpdateRecord } from "app/common/DocActions"; +import { AddRecord, BulkAddRecord, BulkRemoveRecord, BulkUpdateRecord, + CellValue, DocAction, getTableId, RemoveRecord, ReplaceTableData, + TableDataAction, UpdateRecord } from "app/common/DocActions"; import { getSetMapValue } from "app/common/gutil"; /** @@ -78,7 +79,7 @@ export function getRowIdsFromDocAction(docActions: RemoveRecord | BulkRemoveReco } /** - * Tiny helper to get the row ids mentioned in a record-related DocAction as a list + * 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 ["*"]. */ @@ -88,3 +89,21 @@ export function getColIdsFromDocAction(docActions: RemoveRecord | BulkRemoveReco 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/Sharing.ts b/app/server/lib/Sharing.ts index 460d7392..d5feea87 100644 --- a/app/server/lib/Sharing.ts +++ b/app/server/lib/Sharing.ts @@ -6,6 +6,7 @@ import { LocalActionBundle, UserActionBundle } from 'app/common/ActionBundle'; +import {ApplyUAExtendedOptions} from 'app/common/ActiveDocAPI'; import {CALCULATING_USER_ACTIONS, DocAction, getNumRows, UserAction} from 'app/common/DocActions'; import {allToken} from 'app/common/sharing'; import log from 'app/server/lib/log'; @@ -195,15 +196,16 @@ export class Sharing { const userActions: UserAction[] = [ ['ApplyDocActions', action.stored.map(envContent => envContent[1])] ]; - return this._doApplyUserActions(action.info[1], userActions, Branch.Shared, null); + return this._doApplyUserActions(action.info[1], userActions, Branch.Shared, null, null); } private _doApplyUserActionBundle(action: UserActionBundle, docSession: OptDocSession|null): Promise { - return this._doApplyUserActions(action.info, action.userActions, Branch.Local, docSession); + return this._doApplyUserActions(action.info, action.userActions, Branch.Local, docSession, action.options || null); } private async _doApplyUserActions(info: ActionInfo, userActions: UserAction[], - branch: Branch, docSession: OptDocSession|null): Promise { + branch: Branch, docSession: OptDocSession|null, + options: ApplyUAExtendedOptions|null): Promise { const client = docSession && docSession.client; if (docSession?.linkId) { @@ -211,7 +213,7 @@ export class Sharing { } const {sandboxActionBundle, undo, accessControl} = - await this._modificationLock.runExclusive(() => this._applyActionsToDataEngine(docSession, userActions)); + await this._modificationLock.runExclusive(() => this._applyActionsToDataEngine(docSession, userActions, options)); try { @@ -389,7 +391,8 @@ export class Sharing { shortDesc(envAction[1]))); } - private async _applyActionsToDataEngine(docSession: OptDocSession|null, userActions: UserAction[]) { + private async _applyActionsToDataEngine(docSession: OptDocSession|null, userActions: UserAction[], + options: ApplyUAExtendedOptions|null) { const sandboxActionBundle = await this._activeDoc.applyActionsToDataEngine(docSession, userActions); const undo = getEnvContent(sandboxActionBundle.undo); const docActions = getEnvContent(sandboxActionBundle.stored).concat( @@ -397,7 +400,8 @@ export class Sharing { const isDirect = getEnvContent(sandboxActionBundle.direct); const accessControl = this._activeDoc.getGranularAccessForBundle( - docSession || makeExceptionalDocSession('share'), docActions, undo, userActions, isDirect + docSession || makeExceptionalDocSession('share'), docActions, undo, userActions, isDirect, + options ); try { // TODO: see if any of the code paths that have no docSession are relevant outside