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

@@ -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_')) {

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,28 +914,28 @@ 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 ],
       manualSort: [ 1, 2 ],
       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' ],
     });
   });