From 3e7bdd40fa5efb89d697ca336764471a013c701f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Rahir?= Date: Fri, 13 Jan 2023 10:48:28 +0000 Subject: [PATCH] [FIX] spreadsheet: Catch access errors on spreadsheet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, a user opening a spreadsheet containing pivots/lists/graphs that they don't have the rights to access (be it because of the parent model or the domain applied) will end up in an infinite loop. While starting the datasources, the fetch step will throw, potentially spamming the user with access errors. Manual forward port of https://github.com/odoo/enterprise/pull/35146 Task 3107650 closes odoo/odoo#110113 X-original-commit: 1555e79be0b52c30df43e8de50f5db56fbbda630 Signed-off-by: Rémi Rahir (rar) Signed-off-by: Pierre Rousseau (pro) --- .../chart/data_source/chart_data_source.js | 3 ++ .../static/src/data_sources/data_source.js | 24 ++++++++--- .../charts/model/odoo_chart_plugin_test.js | 34 +++++++++++++++- .../tests/data_fetching/data_source_test.js | 28 +++++++++++++ .../static/tests/lists/list_plugin_test.js | 40 +++++++++++++++++++ .../tests/pivots/model/pivot_plugin_test.js | 39 ++++++++++++++++++ .../spreadsheet/static/tests/utils/model.js | 1 + 7 files changed, 163 insertions(+), 6 deletions(-) diff --git a/addons/spreadsheet/static/src/chart/data_source/chart_data_source.js b/addons/spreadsheet/static/src/chart/data_source/chart_data_source.js index 01c27d6d3c8..7a3fe89c0a1 100644 --- a/addons/spreadsheet/static/src/chart/data_source/chart_data_source.js +++ b/addons/spreadsheet/static/src/chart/data_source/chart_data_source.js @@ -39,6 +39,9 @@ export default class ChartDataSource extends OdooViewsDataSource { this.load(); return { datasets: [], labels: [] }; } + if (!this._isValid) { + return { datasets: [], labels: [] }; + } return this._model.data; } } diff --git a/addons/spreadsheet/static/src/data_sources/data_source.js b/addons/spreadsheet/static/src/data_sources/data_source.js index f6142e96599..b9259d95381 100644 --- a/addons/spreadsheet/static/src/data_sources/data_source.js +++ b/addons/spreadsheet/static/src/data_sources/data_source.js @@ -1,6 +1,7 @@ /** @odoo-module */ import { LoadingDataError } from "@spreadsheet/o_spreadsheet/errors"; +import { RPCError } from "@web/core/network/rpc_service"; import { KeepLast } from "@web/core/utils/concurrency"; /** @@ -30,6 +31,8 @@ export class LoadableDataSource { */ this._loadPromise = undefined; this._isFullyLoaded = false; + this._isValid = true; + this._loadErrorMessage = ""; } /** @@ -45,11 +48,19 @@ export class LoadableDataSource { } if (!this._loadPromise) { this._isFullyLoaded = false; - this._loadPromise = this._concurrency.add(this._load()).then(() => { - this._lastUpdate = Date.now(); - this._isFullyLoaded = true; - this._notify(); - }); + this._isValid = true; + this._loadErrorMessage = ""; + this._loadPromise = this._concurrency + .add(this._load()) + .catch((e) => { + this._isValid = false; + this._loadErrorMessage = e instanceof RPCError ? e.data.message : e.message; + }) + .finally(() => { + this._lastUpdate = Date.now(); + this._isFullyLoaded = true; + this._notify(); + }); } return this._loadPromise; } @@ -73,6 +84,9 @@ export class LoadableDataSource { this.load(); throw new LoadingDataError(); } + if (!this._isValid) { + throw new Error(this._loadErrorMessage); + } } /** diff --git a/addons/spreadsheet/static/tests/charts/model/odoo_chart_plugin_test.js b/addons/spreadsheet/static/tests/charts/model/odoo_chart_plugin_test.js index b0c5ed35e9f..1a3d07e574a 100644 --- a/addons/spreadsheet/static/tests/charts/model/odoo_chart_plugin_test.js +++ b/addons/spreadsheet/static/tests/charts/model/odoo_chart_plugin_test.js @@ -5,8 +5,9 @@ import { OdooChart } from "@spreadsheet/chart/odoo_chart/odoo_chart"; import { OdooLineChart } from "@spreadsheet/chart/odoo_chart/odoo_line_chart"; import { nextTick } from "@web/../tests/helpers/utils"; import { createSpreadsheetWithChart, insertChartInSpreadsheet } from "../../utils/chart"; -import { createModelWithDataSource } from "../../utils/model"; +import { createModelWithDataSource, waitForDataSourcesLoaded } from "../../utils/model"; import spreadsheet from "@spreadsheet/o_spreadsheet/o_spreadsheet_extended"; +import { RPCError } from "@web/core/network/rpc_service"; const { toZone } = spreadsheet.helpers; @@ -398,4 +399,35 @@ QUnit.module("spreadsheet > odoo chart plugin", {}, () => { model.getters.getChartRuntime(chartId).chartJsConfig.options.scales.yAxes[0].stacked ); }); + + QUnit.test( + "Load odoo chart spreadsheet with models that cannot be accessed", + async function (assert) { + let hasAccessRights = true; + const { model } = await createSpreadsheetWithChart({ + mockRPC: async function (route, args) { + if ( + args.model === "partner" && + args.method === "web_read_group" && + !hasAccessRights + ) { + const error = new RPCError(); + error.data = { message: "ya done!" }; + throw error; + } + }, + }); + const chartId = model.getters.getFigures(model.getters.getActiveSheetId())[0].id; + const chartDataSource = model.getters.getChartDataSource(chartId); + await waitForDataSourcesLoaded(model); + const data = chartDataSource.getData(); + assert.equal(data.datasets.length, 1); + assert.equal(data.labels.length, 2); + + hasAccessRights = false; + chartDataSource.load({ reload: true }); + await waitForDataSourcesLoaded(model); + assert.deepEqual(chartDataSource.getData(), { datasets: [], labels: [] }); + } + ); }); diff --git a/addons/spreadsheet/static/tests/data_fetching/data_source_test.js b/addons/spreadsheet/static/tests/data_fetching/data_source_test.js index 6c5cfbd4b6e..7af839011d8 100644 --- a/addons/spreadsheet/static/tests/data_fetching/data_source_test.js +++ b/addons/spreadsheet/static/tests/data_fetching/data_source_test.js @@ -3,6 +3,7 @@ import { nextTick } from "@web/../tests/helpers/utils"; import { LoadableDataSource } from "@spreadsheet/data_sources/data_source"; import { Deferred } from "@web/core/utils/concurrency"; +import { RPCError } from "@web/core/network/rpc_service"; QUnit.module("spreadsheet data source", {}, () => { QUnit.test( @@ -43,4 +44,31 @@ QUnit.module("spreadsheet data source", {}, () => { assert.strictEqual(dataSource.isReady(), true); } ); + + QUnit.test("Datasources handle errors thrown at _load", async (assert) => { + class TestDataSource extends LoadableDataSource { + constructor() { + super(...arguments); + this.data = null; + } + async _load() { + this.data = await this._orm.call(); + } + } + + const dataSource = new TestDataSource({ + notify: () => {}, + orm: { + call: () => { + const error = new RPCError(); + error.data = { message: "Ya done!" }; + throw error; + }, + }, + }); + await dataSource.load(); + assert.ok(dataSource._isFullyLoaded); + assert.notOk(dataSource._isValid); + assert.equal(dataSource._loadErrorMessage, "Ya done!"); + }); }); diff --git a/addons/spreadsheet/static/tests/lists/list_plugin_test.js b/addons/spreadsheet/static/tests/lists/list_plugin_test.js index ec9b0a08ad1..b6329bdbda2 100644 --- a/addons/spreadsheet/static/tests/lists/list_plugin_test.js +++ b/addons/spreadsheet/static/tests/lists/list_plugin_test.js @@ -16,6 +16,7 @@ import { } from "../utils/getters"; import { createSpreadsheetWithList } from "../utils/list"; import { registry } from "@web/core/registry"; +import { RPCError } from "@web/core/network/rpc_service"; QUnit.module("spreadsheet > list plugin", {}, () => { QUnit.test("List export", async (assert) => { @@ -506,4 +507,43 @@ QUnit.module("spreadsheet > list plugin", {}, () => { }, }); }); + + QUnit.test( + "Load list spreadsheet with models that cannot be accessed", + async function (assert) { + let hasAccessRights = true; + const { model } = await createSpreadsheetWithList({ + mockRPC: async function (route, args) { + if ( + args.model === "partner" && + args.method === "search_read" && + !hasAccessRights + ) { + const error = new RPCError(); + error.data = { message: "ya done!" }; + throw error; + } + }, + }); + let headerCell; + let cell; + await waitForDataSourcesLoaded(model); + headerCell = getEvaluatedCell(model, "A3"); + cell = getEvaluatedCell(model, "C3"); + + assert.equal(headerCell.value, 1); + assert.equal(cell.value, 42669); + + hasAccessRights = false; + model.dispatch("REFRESH_ODOO_LIST", { listId: "1" }); + await waitForDataSourcesLoaded(model); + headerCell = getEvaluatedCell(model, "A3"); + cell = getEvaluatedCell(model, "C3"); + + assert.equal(headerCell.value, "#ERROR"); + assert.equal(headerCell.error.message, "ya done!"); + assert.equal(cell.value, "#ERROR"); + assert.equal(cell.error.message, "ya done!"); + } + ); }); diff --git a/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js b/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js index a9013fb7f08..245aea42a78 100644 --- a/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js +++ b/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js @@ -16,6 +16,7 @@ import { } from "@spreadsheet/../tests/utils/model"; import { makeDeferred, nextTick, patchWithCleanup } from "@web/../tests/helpers/utils"; import { session } from "@web/session"; +import { RPCError } from "@web/core/network/rpc_service"; QUnit.module("spreadsheet > pivot plugin", {}, () => { QUnit.test("can select a Pivot from cell formula", async function (assert) { @@ -715,4 +716,42 @@ QUnit.module("spreadsheet > pivot plugin", {}, () => { assert.deepEqual(model.getters.getPivotFieldMatching("1", filter.id), undefined); assert.deepEqual(model.getters.getPivotDataSource("1").getComputedDomain(), []); }); + + QUnit.test( + "Load pivot spreadsheet with models that cannot be accessed", + async function (assert) { + let hasAccessRights = true; + const { model } = await createSpreadsheetWithPivot({ + mockRPC: async function (route, args) { + if ( + args.model === "partner" && + args.method === "read_group" && + !hasAccessRights + ) { + const error = new RPCError(); + error.data = { message: "ya done!" }; + throw error; + } + }, + }); + let headerCell; + let cell; + + await waitForDataSourcesLoaded(model); + headerCell = getEvaluatedCell(model, "A3"); + cell = getEvaluatedCell(model, "C3"); + assert.equal(headerCell.value, "No"); + assert.equal(cell.value, 15); + + hasAccessRights = false; + model.dispatch("REFRESH_PIVOT", { id: "1" }); + await waitForDataSourcesLoaded(model); + headerCell = getEvaluatedCell(model, "A3"); + cell = getEvaluatedCell(model, "C3"); + assert.equal(headerCell.value, "#ERROR"); + assert.equal(headerCell.error.message, "ya done!"); + assert.equal(cell.value, "#ERROR"); + assert.equal(cell.error.message, "ya done!"); + } + ); }); diff --git a/addons/spreadsheet/static/tests/utils/model.js b/addons/spreadsheet/static/tests/utils/model.js index eba98b62b74..37e37169ead 100644 --- a/addons/spreadsheet/static/tests/utils/model.js +++ b/addons/spreadsheet/static/tests/utils/model.js @@ -13,6 +13,7 @@ const { Model } = spreadsheet; /** * @typedef {import("@spreadsheet/../tests/utils/data").ServerData} ServerData + * @typedef {import("@spreadsheet/o_spreadsheet/o_spreadsheet").Model} Model */ export function setupDataSourceEvaluation(model) {