From 7907467dbc20cfbec15534f43259d2df5234510d Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 15 Sep 2021 16:18:00 -0400 Subject: [PATCH] (core) treat summary tables like formulas for access control purposes Summary: This unsets the `direct` flag for actions emitted when summary tables are updated. That means those actions will be ignored for access control purposes. So if a user has the right to change a source table, the resulting changes to the summary won't result in the overall action bundle being forbidden. I don't think I've actually seen the use case that inspired this issue being filed. I could imagine perhaps a user forbidden from creating rows globally making permitted updates that could add rows in a summary (and it being desirable to allow that). Test Plan: added tests Reviewers: jarek Reviewed By: jarek Subscribers: dsagal, alexmojaki, jarek Differential Revision: https://phab.getgrist.com/D3022 --- app/common/ActiveDocAPI.ts | 12 +++++++++--- app/common/UserAPI.ts | 9 +++++---- sandbox/grist/table.py | 18 ++++++++++++++---- sandbox/grist/useractions.py | 22 +++++++++++++++++++++- 4 files changed, 49 insertions(+), 12 deletions(-) diff --git a/app/common/ActiveDocAPI.ts b/app/common/ActiveDocAPI.ts index 8c26c94a..9dbd9948 100644 --- a/app/common/ActiveDocAPI.ts +++ b/app/common/ActiveDocAPI.ts @@ -78,9 +78,7 @@ export interface ImportOptions { */ interface BaseQuery { tableId: string; - filters: { - [colId: string]: any[]; - }; + filters: QueryFilters; } /** @@ -101,6 +99,14 @@ export interface ServerQuery extends BaseQuery { limit?: number; } +/** + * Type of the filters option to queries. + */ +export interface QueryFilters { + // TODO: check if "any" can be replaced with "CellValue". + [colId: string]: any[]; +} + export type QueryOperation = "in" | "intersects"; /** diff --git a/app/common/UserAPI.ts b/app/common/UserAPI.ts index 8c4d0519..c991352c 100644 --- a/app/common/UserAPI.ts +++ b/app/common/UserAPI.ts @@ -1,5 +1,5 @@ import {ActionSummary} from 'app/common/ActionSummary'; -import {ApplyUAResult} from 'app/common/ActiveDocAPI'; +import {ApplyUAResult, QueryFilters} from 'app/common/ActiveDocAPI'; import {BaseAPI, IOptions} from 'app/common/BaseAPI'; import {BillingAPI, BillingAPIImpl} from 'app/common/BillingAPI'; import {BrowserSettings} from 'app/common/BrowserSettings'; @@ -337,7 +337,7 @@ export interface UserAPI { * reasons, such as downloads. */ export interface DocAPI { - getRows(tableId: string): Promise; + getRows(tableId: string, options?: { filters?: QueryFilters }): Promise; updateRows(tableId: string, changes: TableColValues): Promise; addRows(tableId: string, additions: BulkColValues): Promise; removeRows(tableId: string, removals: number[]): Promise; @@ -728,8 +728,9 @@ export class DocAPIImpl extends BaseAPI implements DocAPI { this._url = `${url}/api/docs/${docId}`; } - public async getRows(tableId: string): Promise { - return this.requestJson(`${this._url}/tables/${tableId}/data`); + public async getRows(tableId: string, options?: { filters?: QueryFilters }): Promise { + const query = options?.filters ? ("?filter=" + encodeURIComponent(JSON.stringify(options.filters))) : ''; + return this.requestJson(`${this._url}/tables/${tableId}/data${query}`); } public async updateRows(tableId: string, changes: TableColValues): Promise { diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index ceca65fd..483d4006 100644 --- a/sandbox/grist/table.py +++ b/sandbox/grist/table.py @@ -294,7 +294,12 @@ class Table(object): if summary_table._summary_simple: @usertypes.formulaType(usertypes.Reference(summary_table.table_id)) def _updateSummary(rec, table): # pylint: disable=unused-argument - return summary_table.lookupOrAddDerived(**{c: getattr(rec, c) for c in groupby_cols}) + try: + # summary table output should be treated as we treat formula columns, for acl purposes + self._engine.user_actions.enter_indirection() + return summary_table.lookupOrAddDerived(**{c: getattr(rec, c) for c in groupby_cols}) + finally: + self._engine.user_actions.leave_indirection() else: @usertypes.formulaType(usertypes.ReferenceList(summary_table.table_id)) def _updateSummary(rec, table): # pylint: disable=unused-argument @@ -333,9 +338,14 @@ class Table(object): new_row_ids.append(None) if new_row_ids and not self._engine.is_triggered_by_table_action(summary_table.table_id): - result += self._engine.user_actions.BulkAddRecord( - summary_table.table_id, new_row_ids, values_to_add - ) + try: + # summary table output should be treated as we treat formula columns, for acl purposes + self._engine.user_actions.enter_indirection() + result += self._engine.user_actions.BulkAddRecord( + summary_table.table_id, new_row_ids, values_to_add + ) + finally: + self._engine.user_actions.leave_indirection() return result diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 7562d9d6..c390bf9f 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -30,6 +30,11 @@ log = logger.Logger(__name__, logger.INFO) _current_module = sys.modules[__name__] _action_types = {} +# When distinguishing actions directly requested by the user from actions that +# are indirect consequences of those actions (specifically, adding rows to summary tables) +# we count levels of indirection. Zero indirection levels implies an action is directly +# requested. +DIRECT_ACTION = 0 # Fields of _grist_Tables_column table that may be modified using ModifyColumns useraction. _modifiable_col_fields = {'type', 'widgetOptions', 'formula', 'isFormula', 'label', @@ -147,19 +152,34 @@ class UserActions(object): self._summary = summary.SummaryActions(self, self._docmodel) self._import_actions = import_actions.ImportActions(self, self._docmodel, eng) self._allow_changes = False + self._indirection_level = DIRECT_ACTION # Map of methods implementing particular (action_name, table_id) combinations. It mirrors # global _action_method_overrides, but with methods *bound* to this UserActions instance. self._overrides = {key: method.__get__(self, UserActions) for key, method in six.iteritems(_action_method_overrides)} + def enter_indirection(self): + """ + Mark any actions following this call as being indirect, until leave_indirection is + called. Nesting is supported (but not used). + """ + self._indirection_level += 1 + + def leave_indirection(self): + """ + Undo an enter_indirection. + """ + self._indirection_level -= 1 + assert self._indirection_level >= 0 + def _do_doc_action(self, action): if hasattr(action, 'simplify'): # Convert bulk actions to single actions if possible, or None if it affects no rows. action = action.simplify() if action: self._engine.out_actions.stored.append(action) - self._engine.out_actions.direct.append(True) + self._engine.out_actions.direct.append(self._indirection_level == DIRECT_ACTION) self._engine.apply_doc_action(action) def _bulk_action_iter(self, table_id, row_ids, col_values=None):