Archives
/
gristlabs_grist-core
mirror of https://github.com/gristlabs/grist-core
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

(core) allow undos to be partial, if access control prohibits some part of them

Browse Source 
Summary:
This is a somewhat experimental change, that will implement permitted parts of an undo if not all parts are permitted.  This is in preparation for trigger columns, where it may become common for a change in a record resulting in a change to an automatic change to another that the user cannot edit directly.  How to undo such an action is somewhat unclear.  One option is to undo the permitted parts, and then the triggers can rerun.

The general case is a bit of a can of worms, and feels adjacent to merging/rebasing etc.

Oh: it would probably be important in general to communicate to the user that an undo was partial, but this diff doesn't do that.  It would need some new plumbing.

Test Plan: added test

Reviewers: dsagal

Reviewed By: dsagal

Differential Revision: https://phab.getgrist.com/D2839
pull/9/head
Paul Fitzpatrick 3 years ago
parent af76c11be6
commit 6f02987d10
4 changed files with 277 additions and 11 deletions
Show Stats Download Patch File Download Diff File

1
app/common/ActiveDocAPI.ts
Unescape View File

@ -10,6 +10,7 @@ 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.
}
export interface ApplyUAResult {

5
app/common/gristTypes.ts
Unescape View File

@ -20,6 +20,7 @@ export const enum GristObjCode {
DateTime = 'D',
Date = 'd',
Skip = 'S',
Censored = 'C',
Reference = 'R',
Exception = 'E',
Pending = 'P',
@ -131,6 +132,10 @@ export function isSkip(value: CellValue): value is [GristObjCode.Skip] {
return getObjCode(value) === GristObjCode.Skip;
}
export function isCensored(value: CellValue): value is [GristObjCode.Censored] {
return getObjCode(value) === GristObjCode.Censored;
}
/**
* Returns whether a value (as received in a DocAction) represents a list or is null,
* which is a valid value for list types in grist.

6
app/server/lib/ActiveDoc.ts
Unescape View File

@ -783,7 +783,8 @@ export class ActiveDoc extends EventEmitter {
// Granular access control implemented ultimately in _applyUserActions.
// It could be that error cases and timing etc leak some info prior to this
// point.
return this.applyUserActions(docSession, actions, options);
// Undos are best effort now by default.
return this.applyUserActions(docSession, actions, {bestEffort: undo, ...(options||{})});
}
/**
@ -1170,6 +1171,9 @@ export class ActiveDoc extends EventEmitter {
this.logDebug(docSession, "_applyUserActions(%s, %s)", client, shortDesc(actions));
this._inactivityTimer.ping(); // The doc is in active use; ping it to stay open longer.
if (options?.bestEffort) {
actions = await this._granularAccess.prefilterUserActions(docSession, actions);
}
await this._granularAccess.assertCanMaybeApplyUserActions(docSession, actions);
const user = docSession.mode === 'system' ? 'grist' :

276
app/server/lib/GranularAccess.ts
Unescape View File

@ -13,6 +13,7 @@ import { UserOverride } from 'app/common/DocListAPI';
import { ErrorWithCode } from 'app/common/ErrorWithCode';
import { AclMatchInput, InfoEditor, InfoView } from 'app/common/GranularAccessClause';
import { UserInfo } from 'app/common/GranularAccessClause';
import { isCensored } from 'app/common/gristTypes';
import { getSetMapValue, isObject, pruneArray } from 'app/common/gutil';
import { canView, Role } from 'app/common/roles';
import { FullUser } from 'app/common/UserAPI';
@ -44,14 +45,19 @@ function isDataAction(a: UserAction): a is DataAction {
return ACTION_WITH_TABLE_ID.has(String(a[0]));
}
function isAddRecordAction(a: DataAction): boolean {
function isAddRecordAction(a: DataAction): a is AddRecord | BulkAddRecord {
return ['AddRecord', 'BulkAddRecord'].includes(a[0]);
}
function isRemoveRecordAction(a: DataAction): boolean {
function isRemoveRecordAction(a: DataAction): a is RemoveRecord | BulkRemoveRecord {
return ['RemoveRecord', 'BulkRemoveRecord'].includes(a[0]);
}
function isBulkAction(a: DataAction): a is BulkAddRecord | BulkUpdateRecord |
BulkRemoveRecord | ReplaceTableData | TableDataAction {
return Array.isArray(a[2]);
}
// Check if a tableId is that of an ACL table. Currently just _grist_ACLRules and
// _grist_ACLResources are accepted.
function isAclTable(tableId: string): boolean {
@ -381,6 +387,89 @@ export class GranularAccess implements GranularAccessForBundle {
return canCertainlyApply;
}
/**
* Called when it is permissible to partially fulfill the requested actions.
* Will remove forbidden actions in a very limited set of recognized circumstances.
* In fact, currently in only one circumstance:
*
* - If there is a single requested action, and it is an ApplyUndoActions.
* The goal being to let a user undo their action to the extent that it
* is possible to do so.
*
* In this case, the list of actions nested in ApplyUndoActions will be extracted,
* treated as DocActions, and filtered to remove any component parts (at action,
* column, row, or individual cell level) that would be forbidden.
*
* Beyond pure data changes, there are no heroics - any schema change will
* result in prefiltering being skipped.
*
* 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<UserAction[]> {
// Currently we only attempt prefiltering for an ApplyUndoActions.
if (actions.length !== 1) { return actions; }
const userAction = actions[0];
if (userAction[0] !== 'ApplyUndoActions') { return actions; }
// Ok, this is an undo. Unpack the requested undo actions. For a bona
// fide ApplyUndoActions, these would be doc actions generated by the
// data engine and stored in action history. But there is no actual
// restriction in how ApplyUndoActions could be generated. Security
// is enforced separately, so we don't need to be paranoid here.
const docActions = userAction[1] as DocAction[];
// Bail out if there is any hint of a schema change.
// TODO: may want to also bail if an action we'd need to filter would
// affect a row id used later in the bundle. Perhaps prefiltering
// should be restricted to bundles of updates only for that reason.
for (const action of docActions) {
if (!isDataAction(action) || getTableId(action).startsWith('_grist')) {
return actions;
}
}
// Run through a simulation of access control on these actions,
// retaining only permitted material.
const proposedActions: UserAction[] = [];
try {
// Establish our doc actions as the current context for access control.
// We don't have undo information for them, but don't need to because
// they have not been applied to the db. Treat all actions as "direct"
// since we could not trust claims of indirectness currently in
// any case (though we could rearrange to limit how undo actions are
// requested).
this.getGranularAccessForBundle(docSession, docActions, [], docActions,
docActions.map(() => true));
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
// single action into a series of actions.
try {
await this._checkIncomingDocAction({docSession, action, actionIdx});
// Nothing forbidden! Keep this action unchanged.
proposedActions.push(action);
} catch (e) {
if (String(e.code) !== 'ACL_DENY') { throw e; }
const acts = await this._prefilterDocAction({docSession, action, actionIdx});
proposedActions.push(...acts);
// Presumably we've changed the action. Zap our cache of intermediate
// states, since it is stale now. TODO: reorganize cache to so can avoid wasting
// time repeating work unnecessarily. The cache was designed with all-or-nothing
// operations in mind, and is poorly suited to prefiltering.
// Note: the meaning of newRec is slippery in prefiltering, since it depends on
// state at the end of the bundle, but that state is unstable now.
// TODO look into prefiltering in cases using newRec in a many-action bundle.
this._steps = null;
this._metaSteps = null;
}
}
} finally {
await this.finishedBundle();
}
return [['ApplyUndoActions', proposedActions]];
}
/**
* Check if user may be able to apply a given action. Throws if
* user cannot apply the action. Returns true if a user can apply an
@ -566,7 +655,7 @@ export class GranularAccess implements GranularAccessForBundle {
const cursor: ActionCursor = {docSession, action: data, actionIdx: null};
const tableId = getTableId(data);
if (this.getReadPermission(permInfo.getTableAccess(tableId)) === 'mixed') {
await this._filterRowsAndCells(cursor, data, data, readAccessCheck);
await this._filterRowsAndCells(cursor, data, data, readAccessCheck, true);
}
// Filter columns, omitting any to which the user has no access, regardless of rows.
@ -800,7 +889,7 @@ export class GranularAccess implements GranularAccessForBundle {
// Return the results, also applying any cell-level access control.
for (const a of revisedDocActions) {
await this._filterRowsAndCells({...cursor, action: a}, rowsAfter, rowsAfter, readAccessCheck);
await this._filterRowsAndCells({...cursor, action: a}, rowsAfter, rowsAfter, readAccessCheck, false);
}
return revisedDocActions;
}
@ -814,7 +903,7 @@ 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);
await this._filterRowsAndCells(cursor, rowsBefore, rowsAfter, accessCheck, false);
}
private async _getRowsBeforeAndAfter(cursor: ActionCursor) {
@ -856,13 +945,28 @@ 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.
*
* 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
* an error will be thrown. This flag setting is used when filtering outgoing
* actions, where actions need rewriting elsewhere to reflect access changes to
* rows for each individual client.
*/
private async _filterRowsAndCells(cursor: ActionCursor, rowsBefore: TableDataAction, rowsAfter: TableDataAction,
accessCheck: IAccessCheck) {
accessCheck: IAccessCheck,
allowRowRemoval: boolean): Promise<{
filteredAction: DocAction | null,
censoredRows: Set<number>
}> {
const censoredRows = new Set<number>();
const ruler = await this._getRuler(cursor);
const {docSession, action} = cursor;
let filteredAction: DocAction | null = action;
if (action && isSchemaAction(action)) {
return [];
return {filteredAction, censoredRows};
}
// For user convenience, for creations and deletions we equate rec and newRec.
@ -921,14 +1025,19 @@ export class GranularAccess implements GranularAccessForBundle {
const colAccess = rowPermInfo.getColumnAccess(tableId, colId);
if (accessCheck.get(colAccess) === 'deny') {
censorAt(colId, idx);
censoredRows.add(idx);
}
}
}
}
if (toRemove.length > 0) {
if (rowsBefore === action) {
this._removeRowsAt(toRemove, rowsBefore[2], rowsBefore[3]);
if (allowRowRemoval) {
if (Array.isArray(action[2])) {
this._removeRowsAt(toRemove, action[2], action[3]);
} else {
filteredAction = null;
}
} else {
// Artificially introduced removals are ok, otherwise this is suspect.
if (action[0] !== 'RemoveRecord' && action[0] !== 'BulkRemoveRecord') {
@ -936,6 +1045,7 @@ export class GranularAccess implements GranularAccessForBundle {
}
}
}
return {filteredAction, censoredRows};
}
// Compute which of the row ids supplied are for rows forbidden for this session.
@ -975,7 +1085,7 @@ export class GranularAccess implements GranularAccessForBundle {
*
* toRemove must be sorted, lowest to highest.
*/
private _removeRowsAt(toRemove: number[], rowIds: number[], colValues: BulkColValues|undefined) {
private _removeRowsAt(toRemove: number[], rowIds: number[], colValues: BulkColValues|ColValues|undefined) {
if (toRemove.length > 0) {
pruneArray(rowIds, toRemove);
if (colValues) {
@ -1304,6 +1414,63 @@ export class GranularAccess implements GranularAccessForBundle {
return steps;
}
/**
* Return any permitted parts of an action. A completely forbidden
* action results in an empty list. Forbidden columns and rows will
* be stripped from a returned action. Rows with forbidden cells are
* extracted and returned in distinct actions (since they will have
* a distinct set of columns).
*
* This method should only be called with data actions, and will throw
* for anything else.
*/
private async _prefilterDocAction(cursor: ActionCursor): Promise<DocAction[]> {
const {action, docSession} = cursor;
const tableId = getTableId(action);
const permInfo = await this._getStepAccess(cursor);
const tableAccess = permInfo.getTableAccess(tableId);
const accessCheck = await this._getAccessForActionType(docSession, action, 'check');
const access = accessCheck.get(tableAccess);
if (access === 'deny') {
// Filter out this action entirely.
return [];
} else if (access === 'allow') {
// Retain this action entirely.
return [action];
} else if (access === 'mixedColumns') {
// Retain some or all columns entirely.
const act = this._pruneColumns(action, permInfo, tableId, accessCheck);
return act ? [act] : [];
}
// The remainder is the mixed condition.
const {rowsBefore, rowsAfter} = await this._getRowsForRecAndNewRec(cursor);
const {censoredRows, filteredAction} = await this._filterRowsAndCells({...cursor, action: cloneDeep(action)},
rowsBefore, rowsAfter, accessCheck,
true);
if (filteredAction === null) {
return [];
}
if (!isDataAction(filteredAction)) {
throw new Error('_prefilterDocAction called with unexpected action');
}
if (isRemoveRecordAction(filteredAction)) {
// removals do not mention columns or cells, so no further complications.
return [filteredAction];
}
// Strip any forbidden columns.
this._filterColumns(
filteredAction[3],
(colId) => accessCheck.get(permInfo.getColumnAccess(tableId, colId)) !== 'deny');
if (censoredRows.size === 0) {
// no cell censorship, so no further complications.
return [filteredAction];
}
return filterColValues(filteredAction, (idx) => censoredRows.has(idx), isCensored);
}
/**
* Tailor the information about a change reported to a given client. The action passed in
* is never modified. The actions output may differ in the following ways:
@ -1805,3 +1972,92 @@ function scanActionsRecursively(actions: (DocAction|UserAction)[],
}
return false;
}
/**
* Takes an action, and removes certain cells from it. The action
* passed in is modified in place, and also returned as part of a list
* of derived actions.
*
* For a non-bulk action, any cell values that return true for
* shouldFilterCell are removed. For a bulk action, there's no way to
* express that in general in a single action. For a bulk action, for
* any row (identified by row index, not rowId) that returns true for
* shouldFilterRow, we remove cell values based on shouldFilterCell
* and add the row to an action with just the remaining cell values.
*
* This is by no means a general-purpose function. It is used only in
* the implementation of partial undos. If is factored out for
* testing purposes.
*
* This method could be made unnecessary if a way were created to have
* unambiguous "holes" in column value arrays, where values for some
* rows are omitted.
*/
export function filterColValues(action: DataAction,
shouldFilterRow: (idx: number) => boolean,
shouldFilterCell: (value: CellValue) => boolean): DataAction[] {
if (isRemoveRecordAction(action)) {
// removals do not have cells, so nothing to do.
return [action];
}
const colIds = Object.keys(action[3]).sort();
const colValues = action[3];
if (!isBulkAction(action)) {
for (const colId of colIds) {
if (shouldFilterCell((colValues as ColValues)[colId])) {
delete colValues[colId];
}
}
return [action];
}
const rowIds = action[2];
// For bulk operations, censored cells require us to reorganize into a set of actions
// with different columns.
const parts: Map<string, typeof action> = new Map();
let at = 0;
for (let idx = 0; idx < rowIds.length; idx++) {
if (!shouldFilterRow(idx)) {
if (idx !== at) {
// Shuffle columnar data up as we remove rows.
rowIds[at] = rowIds[idx];
for (const colId of colIds) {
(colValues as BulkColValues)[colId][at] = (colValues as BulkColValues)[colId][idx];
}
}
at++;
continue;
}
// Some censored data in this row, so move the row to an action specialized
// for the set of columns this row has.
const keys: string[] = [];
const values: BulkColValues = {};
for (const colId of colIds) {
const value = (colValues as BulkColValues)[colId][idx];
if (!shouldFilterCell(value)) {
values[colId] = [value];
keys.push(colId);
}
}
const mergedKey = keys.join(' ');
const peers = parts.get(mergedKey);
if (!peers) {
parts.set(mergedKey, [action[0], action[1], [rowIds[idx]], values]);
} else {
peers[2].push(rowIds[idx]);
for (const key of keys) {
peers[3][key].push(values[key][0]);
}
}
}
// Truncate columnar data.
rowIds.length = at;
for (const colId of colIds) {
(colValues as BulkColValues)[colId].length = at;
}
// Return all actions, in a consistent order for test purposes.
return [action, ...[...parts.keys()].sort().map(key => parts.get(key)!)];
}

Write Preview
Loading…
Cancel
Save