From d8aacbe3b49088f9f1a925c7e46f55b486240d94 Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Thu, 3 Feb 2022 12:09:59 -0500 Subject: [PATCH] (core) AddOrUpdateRecord user action Summary: New user action as described in https://grist.quip.com/fZSrAnJKgO5j/Add-or-Update-Records-API, with options to allow most of the mentioned possible behaviours. The Python code is due to Alex (as should be obvious from the u in behaviours). Test Plan: Added a unit test. Reviewers: alexmojaki Reviewed By: alexmojaki Differential Revision: https://phab.getgrist.com/D3239 --- app/server/lib/GranularAccess.ts | 120 ++++++++++++++++++++++---- sandbox/grist/acl_formula.py | 2 +- sandbox/grist/actions.py | 2 +- sandbox/grist/test_useractions.py | 139 ++++++++++++++++++++++++++++++ sandbox/grist/useractions.py | 27 ++++++ 5 files changed, 270 insertions(+), 20 deletions(-) diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 925b3f4f..0bdbce02 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -34,7 +34,8 @@ import get = require('lodash/get'); // tslint:disable:no-bitwise -// Actions that add/update/remove/replace rows. +// Actions that add/update/remove/replace rows (DocActions only - UserActions +// may also result in row changes but are not in this list). const ACTION_WITH_TABLE_ID = new Set(['AddRecord', 'BulkAddRecord', 'UpdateRecord', 'BulkUpdateRecord', 'RemoveRecord', 'BulkRemoveRecord', 'ReplaceTableData', 'TableData', @@ -379,27 +380,25 @@ export class GranularAccess implements GranularAccessForBundle { /** * Check if user may be able to apply a list of actions. Throws if - * user cannot apply an action. Returns true if a user can apply an - * action, or false if we need to defer making that determination + * user cannot apply an action. Returns true if a user can perhaps apply an + * action, or false if we know we need to defer making that determination * until the data engine translates the user actions to doc actions. */ public async assertCanMaybeApplyUserActions(docSession: OptDocSession, actions: UserAction[]): Promise { - let canCertainlyApply = true; + if (this._hasExceptionalFullAccess(docSession)) { return true; } + + let canMaybeApply = true; for (const action of actions) { if (!await this.assertCanMaybeApplyUserAction(docSession, action)) { - canCertainlyApply = false; + canMaybeApply = false; break; } } - // 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. - if (!canCertainlyApply && scanActionsRecursively(actions, (a) => this.needEarlySchemaPermission(a))) { - await this._assertSchemaAccess(docSession); - } - return canCertainlyApply; + + await this._checkPossiblePythonFormulaModification(docSession, actions); + await this._checkAddOrUpdateAccess(docSession, actions); + + return canMaybeApply; } /** @@ -493,6 +492,7 @@ export class GranularAccess implements GranularAccessForBundle { */ public async assertCanMaybeApplyUserAction(docSession: OptDocSession, a: UserAction|DocAction): Promise { const name = a[0] as string; + if (this._hasExceptionalFullAccess(docSession)) { return true; } if (OK_ACTIONS.has(name)) { return true; } if (SPECIAL_ACTIONS.has(name)) { if (await this.hasNuancedAccess(docSession)) { @@ -510,6 +510,10 @@ export class GranularAccess implements GranularAccessForBundle { return this.assertCanMaybeApplyUserActions(docSession, a[1] as UserAction[]); } else if (name === 'ApplyDocActions') { return this.assertCanMaybeApplyUserActions(docSession, a[1] as UserAction[]); + } else if (name === 'AddOrUpdateRecord') { + // This case is a bit tricky. + // Access is checked separately in _checkAddOrUpdateAccess. + return true; } else if (isDataAction(a)) { const tableId = getTableId(a); if (tableId.startsWith('_grist_')) { @@ -763,6 +767,61 @@ export class GranularAccess implements GranularAccessForBundle { return result; } + // AddOrUpdateRecord requires broad read access to a table. + // But tables can be renamed, and access can be granted and removed + // within a bundle. + // + // For now, we forbid the combination of AddOrUpdateRecord and + // with actions other than other AddOrUpdateRecords, or simple data + // changes. + // + // Access rules and user attributes might change during the bundle. + // We deny based on access rights at the beginning of the bundle, + // as for _checkPossiblePythonFormulaModification. This is on the + // theory that someone who can change access rights can do anything. + // + // There might be uses for applying AddOrUpdateRecord in a nuanced + // way within the scope of what a user can read, but there's no easy + // way to do that within the data engine as currently + // formulated. Could perhaps be done for on-demand tables though. + private async _checkAddOrUpdateAccess(docSession: OptDocSession, actions: UserAction[]) { + if (!scanActionsRecursively(actions, (a) => a[0] === 'AddOrUpdateRecord')) { + // Don't need to apply this particular check. + return; + } + // Fail if being combined with anything fancy. + if (scanActionsRecursively(actions, (a) => { + const name = a[0]; + return !['ApplyUndoActions', 'ApplyDocActions', 'AddOrUpdateRecord'].includes(String(name)) && + !(isDataAction(a) && !getTableId(a).startsWith('_grist_')); + })) { + throw new Error('Can only combine AddOrUpdate with simple data changes'); + } + // Check for read access, and that we're not touching metadata. + await applyToActionsRecursively(actions, async (a) => { + if (a[0] !== 'AddOrUpdateRecord') { return; } + const tableId = validTableIdString(a[1]); + if (tableId.startsWith('_grist_')) { + throw new Error(`AddOrUpdate cannot yet be used on metadata tables`); + } + const tableAccess = await this.getTableAccess(docSession, tableId); + accessChecks.fatal.read.throwIfNotFullyAllowed(tableAccess); + accessChecks.fatal.update.throwIfDenied(tableAccess); + accessChecks.fatal.create.throwIfDenied(tableAccess); + }); + } + + private async _checkPossiblePythonFormulaModification(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. + if (scanActionsRecursively(actions, (a) => this.needEarlySchemaPermission(a))) { + await this._assertSchemaAccess(docSession); + } + } + /** * Get the role the session user has for this document. User may be overridden, * in which case the role of the override is returned. @@ -1952,6 +2011,7 @@ class UserAttributes { interface IAccessCheck { get(ps: PermissionSetWithContext): string; throwIfDenied(ps: PermissionSetWithContext): void; + throwIfNotFullyAllowed(ps: PermissionSetWithContext): void; } class AccessCheck implements IAccessCheck { @@ -1969,6 +2029,16 @@ class AccessCheck implements IAccessCheck { public throwIfDenied(ps: PermissionSetWithContext): void { const result = ps.perms[this.access]; if (result !== 'deny') { return; } + this._throwError(ps); + } + + public throwIfNotFullyAllowed(ps: PermissionSetWithContext): void { + const result = ps.perms[this.access]; + if (result === 'allow') { return; } + this._throwError(ps); + } + + private _throwError(ps: PermissionSetWithContext): void { const memos = ps.getMemos()[this.access]; const label = this.access === 'schemaEdit' ? 'structure' : @@ -1989,7 +2059,8 @@ export const accessChecks = { // This AccessCheck allows everything. const dummyAccessCheck: IAccessCheck = { get() { return 'allow'; }, - throwIfDenied() {} + throwIfDenied() {}, + throwIfNotFullyAllowed() {} }; @@ -2130,9 +2201,7 @@ function getCensorMethod(tableId: string): (rec: RecordEditor) => void { function scanActionsRecursively(actions: (DocAction|UserAction)[], check: (action: DocAction|UserAction) => boolean): boolean { for (const a of actions) { - if (a[0] === 'ApplyUndoActions') { - return scanActionsRecursively(a[1] as UserAction[], check); - } else if (a[0] === 'ApplyDocActions') { + if (a[0] === 'ApplyUndoActions' || a[0] === 'ApplyDocActions') { return scanActionsRecursively(a[1] as UserAction[], check); } if (check(a)) { return true; } @@ -2140,6 +2209,16 @@ function scanActionsRecursively(actions: (DocAction|UserAction)[], return false; } +async function applyToActionsRecursively(actions: (DocAction|UserAction)[], + op: (action: DocAction|UserAction) => Promise): Promise { + for (const a of actions) { + if (a[0] === 'ApplyUndoActions' || a[0] === 'ApplyDocActions') { + await applyToActionsRecursively(a[1] as UserAction[], op); + } + await op(a); + } +} + /** * Takes an action, and removes certain cells from it. The action * passed in is modified in place, and also returned as part of a list @@ -2266,3 +2345,8 @@ export class User implements UserInfo { return results; } } + +export function validTableIdString(tableId: any): string { + if (typeof tableId !== 'string') { throw new Error(`Expected tableId to be a string`); } + return tableId; +} diff --git a/sandbox/grist/acl_formula.py b/sandbox/grist/acl_formula.py index 17d2bc84..4222aa5b 100644 --- a/sandbox/grist/acl_formula.py +++ b/sandbox/grist/acl_formula.py @@ -51,7 +51,7 @@ def parse_acl_formula_json(acl_formula): # Entities encountered in ACL formulas, which may get renamed. -# type: 'recCol'|'userAttr'|'userAttrCol', +# type : 'recCol'|'userAttr'|'userAttrCol', # start_pos: number, # start position of the token in the code. # name: string, # the name that may be updated by a rename. # extra: string|None, # name of userAttr in case of userAttrCol; otherwise None. diff --git a/sandbox/grist/actions.py b/sandbox/grist/actions.py index 5ad7115a..87fb3797 100644 --- a/sandbox/grist/actions.py +++ b/sandbox/grist/actions.py @@ -49,7 +49,7 @@ ReplaceTableData = namedtuple_eq('ReplaceTableData', BulkAddRecord._fields) # table_id: string name of the table. # col_id: string name of column # col_info: dictionary with particular keys -# type: string type of the column +# type : string type of the column # isFormula: bool, whether it is a formula column # formula: string text of the formula, or empty string # Other keys may be set in col_info (e.g. widgetOptions, label) but are not currently used in diff --git a/sandbox/grist/test_useractions.py b/sandbox/grist/test_useractions.py index 5401af0e..3281aa34 100644 --- a/sandbox/grist/test_useractions.py +++ b/sandbox/grist/test_useractions.py @@ -896,6 +896,145 @@ class TestUserActions(test_engine.EngineTestCase): [2, 2, json.dumps({"included": ["b", "c"]}), None, 1] ]) + def test_add_or_update(self): + sample = testutil.parse_test_sample({ + "SCHEMA": [ + [1, "Table1", [ + [1, "first_name", "Text", False, "", "first_name", ""], + [2, "last_name", "Text", False, "", "last_name", ""], + [3, "pet", "Text", False, "", "pet", ""], + [4, "color", "Text", False, "", "color", ""], + [5, "formula", "Text", True, "''", "formula", ""], + ]], + ], + "DATA": { + "Table1": [ + ["id", "first_name", "last_name"], + [1, "John", "Doe"], + [2, "John", "Smith"], + ], + } + }) + self.load_sample(sample) + + def check(where, values, options, stored): + self.assertPartialOutActions( + self.apply_user_action(["AddOrUpdateRecord", "Table1", where, values, options]), + {"stored": stored}, + ) + + # Exactly one match, so on_many=none has no effect + check( + {"first_name": "John", "last_name": "Smith"}, + {"pet": "dog", "color": "red"}, + {"on_many": "none"}, + [["UpdateRecord", "Table1", 2, {"color": "red", "pet": "dog"}]], + ) + + # Look for a record with pet=dog and change it to pet=cat + check( + {"first_name": "John", "pet": "dog"}, + {"pet": "cat"}, + {}, + [["UpdateRecord", "Table1", 2, {"pet": "cat"}]], + ) + + # Two records match first_name=John, by default we only update the first + check( + {"first_name": "John"}, + {"color": "blue"}, + {}, + [["UpdateRecord", "Table1", 1, {"color": "blue"}]], + ) + + # Update all matching records + check( + {"first_name": "John"}, + {"color": "green"}, + {"on_many": "all"}, + [ + ["UpdateRecord", "Table1", 1, {"color": "green"}], + ["UpdateRecord", "Table1", 2, {"color": "green"}], + ], + ) + + # Don't update any records when there's several matches + check( + {"first_name": "John"}, + {"color": "yellow"}, + {"on_many": "none"}, + [], + ) + + # Invalid value of on_many + with self.assertRaises(ValueError): + check( + {"first_name": "John"}, + {"color": "yellow"}, + {"on_many": "other"}, + [], + ) + + # Since there's at least one matching record and update=False, do nothing + check( + {"first_name": "John"}, + {"color": "yellow"}, + {"update": False}, + [], + ) + + # Since there's no matching records and add=False, do nothing + check( + {"first_name": "John", "last_name": "Johnson"}, + {"first_name": "Jack", "color": "yellow"}, + {"add": False}, + [], + ) + + # No matching record, make a new one. + # first_name=Jack in `values` overrides first_name=John in `where` + check( + {"first_name": "John", "last_name": "Johnson"}, + {"first_name": "Jack", "color": "yellow"}, + {}, + [ + ["AddRecord", "Table1", 3, + {"color": "yellow", "first_name": "Jack", "last_name": "Johnson"}] + ], + ) + + # Specifying a row ID in `where` is allowed + check( + {"first_name": "Bob", "id": 100}, + {"pet": "fish"}, + {}, + [["AddRecord", "Table1", 100, {"first_name": "Bob", "pet": "fish"}]], + ) + + # Now the row already exists + check( + {"first_name": "Bob", "id": 100}, + {"pet": "fish"}, + {}, + [], + ) + + # Nothing matches this `where`, but the row ID already exists + with self.assertRaises(AssertionError): + check( + {"first_name": "Alice", "id": 100}, + {"pet": "fish"}, + {}, + [], + ) + + # Formula columns in `where` can't be used as values when creating records + check( + {"formula": "anything"}, + {"first_name": "Alice"}, + {}, + [["AddRecord", "Table1", 101, {"first_name": "Alice"}]], + ) def test_reference_lookup(self): sample = testutil.parse_test_sample({ diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 3f7fc75e..02702911 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -792,6 +792,33 @@ class UserActions(object): # Otherwise, this is an error. We can't save individual values to formula columns. raise ValueError("Can't save value to formula column %s" % col_id) + @useraction + def AddOrUpdateRecord(self, table_id, where, col_values, options): + table = self._engine.tables[table_id] + records = list(table.lookup_records(**where)) + + if records and options.get("update", True): + if len(records) > 1: + on_many = options.get("on_many", "first") + if on_many == "first": + records = records[:1] + elif on_many == "none": + return + elif on_many != "all": + raise ValueError("on_many should be 'first', 'none', or 'all', not %r" % on_many) + + for record in records: + self.UpdateRecord(table_id, record.id, col_values) + + if not records and options.get("add", True): + values = { + key: value + for key, value in six.iteritems(where) + if not table.get_column(key).is_formula() + } + values.update(col_values) + self.AddRecord(table_id, values.pop("id", None), values) + #---------------------------------------- # RemoveRecords & co. #----------------------------------------