diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 08626869..b90e8104 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -113,8 +113,6 @@ const SPECIAL_ACTIONS = new Set(['InitNewDoc', 'FillTransformRuleColIds', 'TransformAndFinishImport', 'AddView', - 'CopyFromColumn', - 'ConvertFromColumn', 'AddHiddenColumn', 'RespondToRequests', ]); @@ -132,9 +130,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 +145,11 @@ 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. + 'ConvertFromColumn', + 'CopyFromColumn', + // Groups of actions. 'ApplyDocActions', 'ApplyUndoActions', @@ -818,7 +819,7 @@ 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); } @@ -912,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); @@ -1362,7 +1370,6 @@ export class GranularAccess implements GranularAccessForBundle { } await this._assertOnlyBundledWithSimpleDataActions(ADD_OR_UPDATE_RECORD_ACTIONS, actions); - // Check for read access, and that we're not touching metadata. await applyToActionsRecursively(actions, async (a) => { if (!isAddOrUpdateRecordAction(a)) { return; } @@ -1392,12 +1399,15 @@ 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, for example ConvertFromColumn and + // CopyFromColumn. 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 1748df21..87225d2d 100644 --- a/test/server/lib/GranularAccess.ts +++ b/test/server/lib/GranularAccess.ts @@ -457,6 +457,58 @@ describe('GranularAccess', function() { ]); }); + it('respects SCHEMA_EDIT when converting a column', async () => { + // Initially, schema flag defaults to ON for editor. + await freshDoc(); + await owner.applyUserActions(docId, [ + ['AddTable', 'Table1', [{id: 'A', type: 'Int'}, + {id: 'B', type: 'Int'}, + {id: 'C', type: 'Int'}]], + ['AddRecord', '_grist_ACLResources', -1, {tableId: 'Table1', colIds: 'C'}], + // Add at least one access rule. Otherwise the test would succeed + // trivially, via shortcuts in place when the GranularAccess + // hasNuancedAccess test returns false. If there are no access + // rules present, editors can make any edit. Once a granular access + // rule is present, editors lose some rights that are simply too + // hard to compute or we haven't gotten around to. + ['AddRecord', '_grist_ACLRules', null, { + resource: -1, aclFormula: 'user.Access == OWNER', permissionsText: '-R', + }], + ['AddRecord', 'Table1', null, {A: 1234, B: 1234}], + ]); + + // Make a transformation as editor. + await editor.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'}], + ["ConvertFromColumn", "Table1", "A", "gristHelper_Converted", "Text", "", 0], + ["CopyFromColumn", "Table1", "gristHelper_Transform", "A", "{}"], + ]); + + // Now turn off schema flag for editor. + await owner.applyUserActions(docId, [ + ['AddRecord', '_grist_ACLResources', -1, {tableId: '*', colIds: '*'}], + ['AddRecord', '_grist_ACLRules', null, { + resource: -1, aclFormula: 'user.Access == EDITOR', permissionsText: '-S', + }], + ]); + + // Now prepare another transformation. + const transformation = [ + ['AddColumn', 'Table1', 'gristHelper_Converted2', {type: 'Text', isFormula: false, visibleCol: 0, formula: ''}], + ['AddColumn', 'Table1', 'gristHelper_Transform2', + {type: 'Text', isFormula: true, visibleCol: 0, formula: 'rec.gristHelper_Converted2'}], + ["ConvertFromColumn", "Table1", "B", "gristHelper_Converted2", "Text", "", 0], + ["CopyFromColumn", "Table1", "gristHelper_Transform", "B", "{}"], + ]; + // Should fail for editor. + await assert.isRejected(editor.applyUserActions(docId, transformation), + /Blocked by full structure access rules/); + // Should go through if run as owner. + await assert.isFulfilled(owner.applyUserActions(docId, transformation)); + }); + async function applyTransformation(colToHide: string) { await freshDoc(); await owner.applyUserActions(docId, [ @@ -906,12 +958,12 @@ describe('GranularAccess', function() { await assert.isRejected(editor.applyUserActions(docId, [ ['CopyFromColumn', 'Data1', 'A', 'B', {}], - ]), /need uncomplicated access/); + ]), /Blocked by full structure access rules/); await assert.isRejected(editor.applyUserActions(docId, [ ['RenameColumn', 'Data1', 'B', 'B'], ['CopyFromColumn', 'Data1', 'A', 'B', {}], - ]), /need uncomplicated access/); + ]), /Blocked by full structure access rules/); assert.deepEqual(await editor.getDocAPI(docId).getRows('Data1'), { id: [ 1, 2 ],