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

@@ -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,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);
     }

test/server/lib/GranularAccess.ts

@@ -457,6 +457,52 @@ 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'}],
+      ['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 +952,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 ],