make access control for ConvertFromColumn action less brutal (#1111)

Access control for ConvertFromColumn in the presence of access rules had previously been left as a TODO. This change allows the action when the user has schema rights. Because schema rights let you create formulas, they let you read anything, so there is currently no value in nuance here.

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
+// will create.
-// 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.
 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);
     }

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 ],