From 7abd6d4f54079348c6c00bc6770a926b378fb538 Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 17 Jul 2024 15:32:27 -0400 Subject: [PATCH] make access control for ConvertFromColumn action less brutal Access control for ConvertFromColumn in the presence of access rules had been left as a TODO. This change allows the action when the user has broad rights to the table it applies to. --- app/server/lib/GranularAccess.ts | 46 +++++++++++++++++++++++++++---- test/server/lib/GranularAccess.ts | 20 ++++++++++---- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 08626869854..f1997c4333d 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -89,10 +89,16 @@ 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 - @@ -113,8 +119,6 @@ const SPECIAL_ACTIONS = new Set(['InitNewDoc', 'FillTransformRuleColIds', 'TransformAndFinishImport', 'AddView', - 'CopyFromColumn', - 'ConvertFromColumn', 'AddHiddenColumn', 'RespondToRequests', ]); @@ -132,9 +136,7 @@ const OK_ACTIONS = new Set(['Calculate', 'UpdateCurrentTime']); // Only add an action to OTHER_RECOGNIZED_ACTIONS if you know access control // has been handled for it, or it is clear that access control can be done // by looking at the Create/Update/Delete permissions for the DocActions it -// will create. For example, at the time of writing CopyFromColumn should -// not be here, since it could read a column the user is not supposed to -// have access rights to, and it is not handled specially. +// will create. const OTHER_RECOGNIZED_ACTIONS = new Set([ // Data actions. 'AddRecord', @@ -149,6 +151,13 @@ const OTHER_RECOGNIZED_ACTIONS = new Set([ 'AddOrUpdateRecord', '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. + 'ConvertFromColumn', + 'CopyFromColumn', + // Groups of actions. 'ApplyDocActions', 'ApplyUndoActions', @@ -821,6 +830,7 @@ export class GranularAccess implements GranularAccessForBundle { await this._checkPossiblePythonFormulaModification(docSession, actions); await this._checkDuplicateTableAccess(docSession, actions); await this._checkAddOrUpdateAccess(docSession, actions); + await this._checkCopyOrConvertColumnAccess(docSession, actions); } /** @@ -1362,10 +1372,34 @@ 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); + 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 (!isAddOrUpdateRecordAction(a)) { return; } + if (!check(a)) { return; } const actionName = String(a[0]); const tableId = validTableIdString(a[1]); if (tableId.startsWith('_grist_')) { diff --git a/test/server/lib/GranularAccess.ts b/test/server/lib/GranularAccess.ts index 1748df21afa..7a75e0497fb 100644 --- a/test/server/lib/GranularAccess.ts +++ b/test/server/lib/GranularAccess.ts @@ -435,6 +435,8 @@ describe('GranularAccess', function() { isFormula: true, type: 'Any', formula: '', colId: '', widgetOptions: '', label: '', parentPos: 9, parentId: 0 }], + ]); + assert.deepEqual((await cliEditor.readDocUserAction()), [ ['ModifyColumn', 'Table1', 'A', {type: 'Text'}], ['UpdateRecord', 'Table1', 1, {A: '1234' }], ['UpdateRecord', '_grist_Tables_column', 2, {widgetOptions: '{}', type: 'Text'}] @@ -453,6 +455,8 @@ describe('GranularAccess', function() { isFormula: true, type: 'Any', formula: '', colId: '', widgetOptions: '', label: '', parentPos: 9, parentId: 0 }], + ]); + assert.deepEqual((await cliEditor.readDocUserAction()), [ ['UpdateRecord', '_grist_Tables_column', 2, {widgetOptions: '', type: 'Any'}] ]); }); @@ -482,7 +486,9 @@ describe('GranularAccess', function() { await owner.applyUserActions(docId, [ ['AddColumn', 'Table1', 'gristHelper_Converted', {type: 'Text', isFormula: false, visibleCol: 0, formula: ''}], ['AddColumn', 'Table1', 'gristHelper_Transform', - {type: 'Text', isFormula: true, visibleCol: 0, formula: 'rec.gristHelper_Converted'}], + {type: 'Text', isFormula: true, visibleCol: 0, formula: 'rec.gristHelper_Converted'}], + ]); + await owner.applyUserActions(docId, [ // This action is repeated by the UI just before applying (we don't to repeat it here). ["ConvertFromColumn", "Table1", "A", "gristHelper_Converted", "Text", "", 0], ["CopyFromColumn", "Table1", "gristHelper_Transform", "A", "{}"], @@ -516,6 +522,8 @@ describe('GranularAccess', function() { parentPos: 9, parentId: 1 }], + ]); + assert.deepEqual(await cliOwner.readDocUserAction(), [ ['UpdateRecord', 'Table1', 1, {gristHelper_Converted: '1234'}], ['ModifyColumn', 'Table1', 'A', {type: 'Text'}], ['UpdateRecord', 'Table1', 1, {A: '1234'}], @@ -906,12 +914,12 @@ describe('GranularAccess', function() { await assert.isRejected(editor.applyUserActions(docId, [ ['CopyFromColumn', 'Data1', 'A', 'B', {}], - ]), /need uncomplicated access/); + ]), /Blocked by table read access rules/); await assert.isRejected(editor.applyUserActions(docId, [ ['RenameColumn', 'Data1', 'B', 'B'], ['CopyFromColumn', 'Data1', 'A', 'B', {}], - ]), /need uncomplicated access/); + ]), /Can only combine CopyFromColumn and ConvertFromColum/); assert.deepEqual(await editor.getDocAPI(docId).getRows('Data1'), { id: [ 1, 2 ], @@ -919,15 +927,15 @@ describe('GranularAccess', function() { B: [ 'b1', 'b2' ], }); - await assert.isFulfilled(owner.applyUserActions(docId, [ + await assert.isRejected(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: [ 'a1', 'a2' ], + B: [ 'b1', 'b2' ], }); });