From 96d5e44ee7bf279547e49e00b966f43c42354fbc Mon Sep 17 00:00:00 2001 From: "Adrien Minne (adrm)" Date: Mon, 27 Feb 2023 13:20:43 +0000 Subject: [PATCH] [FIX] model: prevent UI plugins to refuse core commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently it's possible for UI plugins to handle and refuse core commands in the allowDispatch. This is a problem because this could lead to de-synchronized states in multi-user. This commit fixes this by making sure that the core commands don't go through the allowDispatch of the UI plugins. Odoo task 3209463 closes odoo/o-spreadsheet#2120 Signed-off-by: Lucas Lefèvre (lul) --- src/model.ts | 19 ++++++++++++++- .../ui_core_views/filter_evaluation.ts | 5 ++-- src/plugins/ui_core_views/sheetview.ts | 3 ++- src/plugins/ui_feature/autofill.ts | 3 ++- src/plugins/ui_feature/cell_popovers.ts | 4 ++-- src/plugins/ui_feature/selection_input.ts | 4 ++-- .../ui_feature/selection_inputs_manager.ts | 4 ++-- src/plugins/ui_feature/sort.ts | 3 ++- src/plugins/ui_feature/ui_sheet.ts | 4 ++-- src/plugins/ui_stateful/clipboard.ts | 3 ++- src/plugins/ui_stateful/edition.ts | 15 ++++++------ src/plugins/ui_stateful/selection.ts | 3 ++- tests/collaborative/collaborative.test.ts | 24 +++++++++++++++++-- tests/model.test.ts | 17 +++++++++++++ 14 files changed, 84 insertions(+), 27 deletions(-) diff --git a/src/model.ts b/src/model.ts index 480b8ae8c..f2a713db4 100644 --- a/src/model.ts +++ b/src/model.ts @@ -46,6 +46,7 @@ import { GridRenderingContext, isCoreCommand, LAYERS, + LocalCommand, UID, } from "./types/index"; import { NotifyUIEvent } from "./types/ui"; @@ -267,7 +268,11 @@ export class Model extends EventBus implements CommandDispatcher { } get handlers(): CommandHandler[] { - return [this.range, ...this.corePlugins, ...this.allUIPlugins, this.history]; + return [...this.coreHandlers, ...this.allUIPlugins, this.history]; + } + + get coreHandlers(): CommandHandler[] { + return [this.range, ...this.corePlugins]; } get allUIPlugins(): UIPlugin[] { @@ -419,6 +424,18 @@ export class Model extends EventBus implements CommandDispatcher { * Check if the given command is allowed by all the plugins and the history. */ private checkDispatchAllowed(command: Command): DispatchResult { + if (isCoreCommand(command)) { + return this.checkDispatchAllowedCoreCommand(command); + } + return this.checkDispatchAllowedLocalCommand(command); + } + + private checkDispatchAllowedCoreCommand(command: CoreCommand): DispatchResult { + const results = this.coreHandlers.map((handler) => handler.allowDispatch(command)); + return new DispatchResult(results.flat()); + } + + private checkDispatchAllowedLocalCommand(command: LocalCommand): DispatchResult { const results = this.handlers.map((handler) => handler.allowDispatch(command)); return new DispatchResult(results.flat()); } diff --git a/src/plugins/ui_core_views/filter_evaluation.ts b/src/plugins/ui_core_views/filter_evaluation.ts index c011838aa..cbc14dfef 100644 --- a/src/plugins/ui_core_views/filter_evaluation.ts +++ b/src/plugins/ui_core_views/filter_evaluation.ts @@ -14,7 +14,6 @@ import { Border, CellPosition, CellValueType, - Command, CommandResult, ExcelFilterData, ExcelWorkbookData, @@ -24,7 +23,7 @@ import { Zone, } from "../../types"; import { UIPlugin } from "../ui_plugin"; -import { UpdateFilterCommand } from "./../../types/commands"; +import { Command, LocalCommand, UpdateFilterCommand } from "./../../types/commands"; export class FilterEvaluationPlugin extends UIPlugin { static getters = [ @@ -41,7 +40,7 @@ export class FilterEvaluationPlugin extends UIPlugin { hiddenRows: Set = new Set(); isEvaluationDirty = false; - allowDispatch(cmd: Command) { + allowDispatch(cmd: LocalCommand): CommandResult { switch (cmd.type) { case "UPDATE_FILTER": if (!this.getters.getFilterId(cmd)) { diff --git a/src/plugins/ui_core_views/sheetview.ts b/src/plugins/ui_core_views/sheetview.ts index 8a9d10684..9ff89497e 100644 --- a/src/plugins/ui_core_views/sheetview.ts +++ b/src/plugins/ui_core_views/sheetview.ts @@ -13,6 +13,7 @@ import { EdgeScrollInfo, Figure, HeaderIndex, + LocalCommand, Pixel, Position, Rect, @@ -117,7 +118,7 @@ export class SheetViewPlugin extends UIPlugin { // Command Handling // --------------------------------------------------------------------------- - allowDispatch(cmd: Command) { + allowDispatch(cmd: LocalCommand): CommandResult | CommandResult[] { switch (cmd.type) { case "SET_VIEWPORT_OFFSET": return this.checkScrollingDirection(cmd); diff --git a/src/plugins/ui_feature/autofill.ts b/src/plugins/ui_feature/autofill.ts index d3d64af3a..eeba56a24 100644 --- a/src/plugins/ui_feature/autofill.ts +++ b/src/plugins/ui_feature/autofill.ts @@ -14,6 +14,7 @@ import { GridRenderingContext, HeaderIndex, LAYERS, + LocalCommand, Tooltip, Zone, } from "../../types/index"; @@ -94,7 +95,7 @@ export class AutofillPlugin extends UIPlugin { // Command Handling // --------------------------------------------------------------------------- - allowDispatch(cmd: Command): CommandResult { + allowDispatch(cmd: LocalCommand): CommandResult { switch (cmd.type) { case "AUTOFILL_SELECT": const sheetId = this.getters.getActiveSheetId(); diff --git a/src/plugins/ui_feature/cell_popovers.ts b/src/plugins/ui_feature/cell_popovers.ts index e386174ff..79728d4f2 100644 --- a/src/plugins/ui_feature/cell_popovers.ts +++ b/src/plugins/ui_feature/cell_popovers.ts @@ -1,6 +1,6 @@ import { positionToZone } from "../../helpers"; import { cellPopoverRegistry } from "../../registries/cell_popovers_registry"; -import { CellPosition, Command, CommandResult, Position, Rect } from "../../types"; +import { CellPosition, Command, CommandResult, LocalCommand, Position, Rect } from "../../types"; import { CellPopoverType, ClosedCellPopover, @@ -20,7 +20,7 @@ export class CellPopoverPlugin extends UIPlugin { private persistentPopover?: CellPosition & { type: CellPopoverType }; - allowDispatch(cmd: Command) { + allowDispatch(cmd: LocalCommand): CommandResult { switch (cmd.type) { case "OPEN_CELL_POPOVER": try { diff --git a/src/plugins/ui_feature/selection_input.ts b/src/plugins/ui_feature/selection_input.ts index ed089b66e..b3fdb8fbb 100644 --- a/src/plugins/ui_feature/selection_input.ts +++ b/src/plugins/ui_feature/selection_input.ts @@ -7,7 +7,7 @@ import { } from "../../helpers/index"; import { StreamCallbacks } from "../../selection_stream/event_stream"; import { SelectionEvent } from "../../types/event_stream"; -import { Command, CommandResult, Highlight, LAYERS, UID } from "../../types/index"; +import { Command, CommandResult, Highlight, LAYERS, LocalCommand, UID } from "../../types/index"; import { UIPlugin, UIPluginConfig } from "../ui_plugin"; export interface RangeInputValue { @@ -50,7 +50,7 @@ export class SelectionInputPlugin extends UIPlugin implements StreamCallbacks { ); }); }); + +test("UI plugins cannot refuse core command and de-synchronize the users", () => { + class MyUIPlugin extends UIPlugin { + allowDispatch(cmd: Command) { + if (cmd.type === "UPDATE_CELL") { + return this.getters.getClient().name === "Alice" + ? CommandResult.Success + : CommandResult.CancelledForUnknownReason; + } + return CommandResult.Success; + } + } + featurePluginRegistry.add("myUIPlugin", MyUIPlugin); + const { alice, bob } = setupCollaborativeEnv(); + + setCellContent(alice, "A1", "hello"); + expect([alice, bob]).toHaveSynchronizedValue((user) => getCellContent(user, "A1"), "hello"); + featurePluginRegistry.remove("myUIPlugin"); +}); diff --git a/tests/model.test.ts b/tests/model.test.ts index 323330f04..e44a92adb 100644 --- a/tests/model.test.ts +++ b/tests/model.test.ts @@ -94,6 +94,23 @@ describe("Model", () => { featurePluginRegistry.remove("myUIPlugin"); }); + test("UI plugins cannot refuse core commands", () => { + class MyUIPlugin extends UIPlugin { + allowDispatch(cmd: Command) { + if (cmd.type === "UPDATE_CELL") { + return CommandResult.CancelledForUnknownReason; + } + return CommandResult.Success; + } + } + featurePluginRegistry.add("myUIPlugin", MyUIPlugin); + const model = new Model(); + + setCellContent(model, "A1", "hello"); + expect(getCellContent(model, "A1")).toBe("hello"); + featurePluginRegistry.remove("myUIPlugin"); + }); + test("Can open a model in readonly mode", () => { const model = new Model({}, { mode: "readonly" }); expect(model.getters.isReadonly()).toBe(true);