From 8c1acacb3d9f6b7f5b8fb3182f195b03be178f95 Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Fri, 19 Jul 2024 10:55:54 -0400 Subject: [PATCH] fall back on schema permission; bundles are too varied and hairy --- app/server/lib/GranularAccess.ts | 63 +++++++++---------------------- test/server/lib/GranularAccess.ts | 10 ++--- 2 files changed, 22 insertions(+), 51 deletions(-) diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 884f8b02c90..89b3360e93b 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -89,16 +89,10 @@ function isAclTable(tableId: string): boolean { const ADD_OR_UPDATE_RECORD_ACTIONS = ['AddOrUpdateRecord', 'BulkAddOrUpdateRecord']; -const COPY_OR_CONVERT_COLUMN_ACTIONS = [ 'CopyFromColumn', 'ConvertFromColumn' ]; - function isAddOrUpdateRecordAction([actionName]: UserAction): boolean { return ADD_OR_UPDATE_RECORD_ACTIONS.includes(String(actionName)); } -function isCopyOrConvertColumnAction([actionName]: UserAction): boolean { - return COPY_OR_CONVERT_COLUMN_ACTIONS.includes(String(actionName)); -} - // A list of key metadata tables that need special handling. Other metadata tables may // refer to material in some of these tables but don't need special handling. // TODO: there are other metadata tables that would need access control, or redesign - @@ -152,9 +146,7 @@ const OTHER_RECOGNIZED_ACTIONS = new Set([ 'BulkAddOrUpdateRecord', // Certain column actions are handled specially because of reads that - // don't fit the pattern of data actions. For these we currently require - // rights on the entire table, and also limit combinations with other - // actions. + // don't fit the pattern of data actions. 'ConvertFromColumn', 'CopyFromColumn', @@ -827,10 +819,9 @@ export class GranularAccess implements GranularAccessForBundle { // Checks are in no particular order. await this._checkSimpleDataActions(docSession, actions); await this._checkForSpecialOrSurprisingActions(docSession, actions); - await this._checkPossiblePythonFormulaModification(docSession, actions); + await this._checkIfNeedsEarlySchemaPermission(docSession, actions); await this._checkDuplicateTableAccess(docSession, actions); await this._checkAddOrUpdateAccess(docSession, actions); - await this._checkCopyOrConvertColumnAccess(docSession, actions); } /** @@ -922,7 +913,14 @@ export class GranularAccess implements GranularAccessForBundle { */ public needEarlySchemaPermission(a: UserAction|DocAction): boolean { const name = a[0] as string; - if (name === 'ModifyColumn' || name === 'SetDisplayFormula') { + if (name === 'ModifyColumn' || name === 'SetDisplayFormula' || + // ConvertFromColumn and CopyFromColumn are hard to reason + // about, especially since they appear in bundles with other + // actions. We throw up our hands a bit here, and just make + // sure the user has schema permissions. Today, in Grist, that + // gives a lot of power. If this gets narrowed down in future, + // we'll have to rethink this. + name === 'ConvertFromColumn' || name === 'CopyFromColumn') { return true; } else if (isDataAction(a)) { const tableId = getTableId(a); @@ -1372,34 +1370,9 @@ export class GranularAccess implements GranularAccessForBundle { } await this._assertOnlyBundledWithSimpleDataActions(ADD_OR_UPDATE_RECORD_ACTIONS, actions); - await this._onlyRegularTablesWithFullTableAccess(docSession, actions, isAddOrUpdateRecordAction); - } - - // CopyFromColumn and ConvertFromColumn actions also read from a - // column. For simplicity, we require access to the full table to - // use them. This could be narrowed down further in future. - // - // But tables can be renamed, and access can be granted and removed - // within a bundle. So for now, we forbid combinations of actions - // that require extra work. - private async _checkCopyOrConvertColumnAccess(docSession: OptDocSession, actions: UserAction[]) { - if (!scanActionsRecursively(actions, isCopyOrConvertColumnAction)) { - // Don't need to apply this particular check. - return; - } - await this._assertOnlyBundledWithSimpleDataActions(COPY_OR_CONVERT_COLUMN_ACTIONS, actions, ['SetDisplayFormula']); - await this._onlyRegularTablesWithFullTableAccess(docSession, actions, isCopyOrConvertColumnAction); - } - - /** - * Only allow a bundle with specific kinds of actions, where we have full - * access to all tables in the bundle. - */ - private async _onlyRegularTablesWithFullTableAccess(docSession: OptDocSession, actions: UserAction[], - check: (action: DocAction|UserAction) => boolean) { // Check for read access, and that we're not touching metadata. await applyToActionsRecursively(actions, async (a) => { - if (!check(a)) { return; } + if (!isAddOrUpdateRecordAction(a)) { return; } const actionName = String(a[0]); const tableId = validTableIdString(a[1]); if (tableId.startsWith('_grist_')) { @@ -1413,14 +1386,10 @@ export class GranularAccess implements GranularAccessForBundle { } /** - * Asserts that `actionNames` (if present in `actions`) are only bundled with simple data actions (or extraActions). + * Asserts that `actionNames` (if present in `actions`) are only bundled with simple data actions. */ - private async _assertOnlyBundledWithSimpleDataActions(actionNames: string | string[], actions: UserAction[], - extraActions?: string[]) { - const names = [ - ...Array.isArray(actionNames) ? actionNames : [actionNames], - ...extraActions ?? [], - ]; + private async _assertOnlyBundledWithSimpleDataActions(actionNames: string | string[], actions: UserAction[]) { + const names = Array.isArray(actionNames) ? actionNames : [actionNames]; // Fail if being combined with anything that isn't a simple data action. await applyToActionsRecursively(actions, async (a) => { const name = String(a[0]); @@ -1430,12 +1399,14 @@ export class GranularAccess implements GranularAccessForBundle { }); } - private async _checkPossiblePythonFormulaModification(docSession: OptDocSession, actions: UserAction[]) { + private async _checkIfNeedsEarlySchemaPermission(docSession: OptDocSession, actions: UserAction[]) { // If changes could include Python formulas, then user must have // +S before we even consider passing these to the data engine. // Since we don't track rule or schema changes at this stage, we // approximate with the user's access rights at beginning of // bundle. + // We also check for +S in scenarios that are hard to break down + // in a more granular way. if (scanActionsRecursively(actions, (a) => this.needEarlySchemaPermission(a))) { await this._assertSchemaAccess(docSession); } diff --git a/test/server/lib/GranularAccess.ts b/test/server/lib/GranularAccess.ts index 7a75e0497fb..beaacf4d19e 100644 --- a/test/server/lib/GranularAccess.ts +++ b/test/server/lib/GranularAccess.ts @@ -914,12 +914,12 @@ describe('GranularAccess', function() { await assert.isRejected(editor.applyUserActions(docId, [ ['CopyFromColumn', 'Data1', 'A', 'B', {}], - ]), /Blocked by table read access rules/); + ]), /Blocked by full structure access rules/); await assert.isRejected(editor.applyUserActions(docId, [ ['RenameColumn', 'Data1', 'B', 'B'], ['CopyFromColumn', 'Data1', 'A', 'B', {}], - ]), /Can only combine CopyFromColumn and ConvertFromColum/); + ]), /Blocked by full structure access rules/); assert.deepEqual(await editor.getDocAPI(docId).getRows('Data1'), { id: [ 1, 2 ], @@ -927,15 +927,15 @@ describe('GranularAccess', function() { B: [ 'b1', 'b2' ], }); - await assert.isRejected(owner.applyUserActions(docId, [ + await assert.isFulfilled(owner.applyUserActions(docId, [ ['RenameColumn', 'Data1', 'B', 'B'], ['CopyFromColumn', 'Data1', 'A', 'B', {}], - ]), /Can only combine CopyFromColumn and ConvertFromColumn/); + ])); assert.deepEqual(await editor.getDocAPI(docId).getRows('Data1'), { id: [ 1, 2 ], manualSort: [ 1, 2 ], - B: [ 'b1', 'b2' ], + B: [ 'a1', 'a2' ], }); });