From b2103b06f71e539c2b65a3e0251ac22ef06eac77 Mon Sep 17 00:00:00 2001 From: Ipsita Borisagar Date: Mon, 12 Dec 2022 11:15:43 +0000 Subject: [PATCH] [FIX] base, web: allow reading property fields when no access to parent record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes a use case where a user would not be able to access the property field values of a record because he did not have access to that record's parent (from the property field definition configuration). In a concrete example: - In the Knowledge App - Marc Demo has access to the "Article B". - The parent article of "Article B" is "Article A". - Marc Demo does not have access to "Article A". - Article B has some property fields values configured. - If Marc Demo tries to open "Article B" -> crash This is caused by the framework having to read the property definition from the parent record, which we don't have access to. A "sudo" was added to allow that use case (which is supported). In addition, this commit also adds further checks on the web client side to gracefully handle a special use case where the end-user could not write on the parent record because of record rules. (It only checked *access rules*). To avoid making an extra RPC every time we load the record (to check if we can write on the parent), we only make that check when the user tries to modify (add/edit/remove) a property and show a proper error message. (This also applies when creating new property field tags). Task-3062064 closes odoo/odoo#118929 X-original-commit: bc538f6944461a642bac0f757ec96d7f8cc14c9a Related: odoo/enterprise#39946 Signed-off-by: Warnon Aurélien (awa) --- addons/web/i18n/web.pot | 15 +++++++ .../fields/properties/properties_field.js | 39 ++++++++++++++++++- .../fields/properties/properties_field.xml | 1 + .../fields/properties/property_definition.js | 1 + .../fields/properties/property_definition.xml | 1 + .../views/fields/properties/property_tags.js | 10 ++++- .../views/fields/properties/property_value.js | 1 + .../fields/properties/property_value.xml | 1 + .../views/fields/properties_field_tests.js | 26 ++++++------- .../test_new_api/tests/test_properties.py | 25 ++++++++---- 10 files changed, 97 insertions(+), 23 deletions(-) diff --git a/addons/web/i18n/web.pot b/addons/web/i18n/web.pot index 8e1dd26e9b1..f5decb24748 100644 --- a/addons/web/i18n/web.pot +++ b/addons/web/i18n/web.pot @@ -5972,6 +5972,21 @@ msgid "" "You may not believe it,
but the application is actually loading..." msgstr "" +#. module: web +#. odoo-javascript +#: code:addons/web/static/src/views/fields/properties/property_tags.js:0 +#, python-format +msgid "You need to be able to edit parent first to add property tags" +msgstr "" + +#. module: web +#. odoo-javascript +#: code:addons/web/static/src/views/fields/properties/properties_field.js:0 +#: code:addons/web/static/src/views/fields/properties/properties_field.js:0 +#, python-format +msgid "You need to be able to edit parent first to configure property fields" +msgstr "" + #. module: web #. odoo-javascript #: code:addons/web/static/src/legacy/js/views/basic/basic_controller.js:0 diff --git a/addons/web/static/src/views/fields/properties/properties_field.js b/addons/web/static/src/views/fields/properties/properties_field.js index 891fb4ea66d..5f94f3e9d02 100644 --- a/addons/web/static/src/views/fields/properties/properties_field.js +++ b/addons/web/static/src/views/fields/properties/properties_field.js @@ -218,9 +218,15 @@ export class PropertiesField extends Component { * @param {event} event * @param {string} propertyName */ - onPropertyEdit(event, propertyName) { + async onPropertyEdit(event, propertyName) { event.stopPropagation(); event.preventDefault(); + if (!await this.checkDefinitionWriteAccess()) { + this.notification.add(_lt("You need to be able to edit parent first to configure property fields"), { + type: "warning", + }); + return; + } if (event.target.classList.contains("disabled")) { // remove the glitch if we click on the edit button // while the popover is already opened @@ -278,7 +284,13 @@ export class PropertiesField extends Component { this.dialogService.add(ConfirmationDialog, dialogProps); } - onPropertyCreate() { + async onPropertyCreate() { + if (!await this.checkDefinitionWriteAccess()) { + this.notification.add(_lt("You need to be able to edit parent first to configure property fields"), { + type: "warning", + }); + return; + } const propertiesDefinitions = this.propertiesList || []; if ( @@ -306,6 +318,28 @@ export class PropertiesField extends Component { this.props.record.update({ [this.props.name]: propertiesDefinitions }); } + /** + * Verify that we can write on properties, + * if we don't have access for parent + */ + async checkDefinitionWriteAccess() { + const definitionRecordId = this.props.record.data[this.definitionRecordField][0]; + const definitionRecordModel = this.props.record.fields[this.definitionRecordField].relation; + try { + await this.orm.call( + definitionRecordModel, + "check_access_rule", + [definitionRecordId], + { + operation: "write", + } + ); + } catch (_e) { // eslint-disable-line no-unused-vars + return false; + } + return true; + } + /** * The tags list has been changed. * If `newValue` is given, update the property value as well. @@ -473,6 +507,7 @@ export class PropertiesField extends Component { this.popover.open(target, { readonly: this.props.readonly || !this.state.canChangeDefinition, canChangeDefinition: this.state.canChangeDefinition, + checkDefinitionWriteAccess: () => this.checkDefinitionWriteAccess(), propertyDefinition: this.propertiesList.find( (property) => property.name === currentName(propertyName) ), diff --git a/addons/web/static/src/views/fields/properties/properties_field.xml b/addons/web/static/src/views/fields/properties/properties_field.xml index 04f78de3fc3..efa1333720f 100644 --- a/addons/web/static/src/views/fields/properties/properties_field.xml +++ b/addons/web/static/src/views/fields/properties/properties_field.xml @@ -31,6 +31,7 @@ diff --git a/addons/web/static/src/views/fields/properties/property_tags.js b/addons/web/static/src/views/fields/properties/property_tags.js index d55ab0719bf..ddfc80732a2 100644 --- a/addons/web/static/src/views/fields/properties/property_tags.js +++ b/addons/web/static/src/views/fields/properties/property_tags.js @@ -188,11 +188,18 @@ export class PropertyTags extends Component { * * @param {string} newLabel */ - onTagCreate(newLabel) { + async onTagCreate(newLabel) { if (!newLabel || !newLabel.length) { return; } + if (!await this.props.checkDefinitionWriteAccess()) { + this.notification.add(_lt("You need to be able to edit parent first to add property tags"), { + type: "warning", + }); + return; + } + const newValue = newLabel ? newLabel.toLowerCase().replace(" ", "_") : ""; const existingTag = this.props.tags.find((tag) => tag[0] === newValue); @@ -292,6 +299,7 @@ PropertyTags.props = { deleteAction: { type: String }, readonly: { type: Boolean, optional: true }, canChangeTags: { type: Boolean, optional: true }, + checkDefinitionWriteAccess: { type: Function, optional: true }, // Select a new value onValueChange: { type: Function, optional: true }, // Change the tags definition (can also receive a second diff --git a/addons/web/static/src/views/fields/properties/property_value.js b/addons/web/static/src/views/fields/properties/property_value.js index 45e4a4786f8..1a5cd0446f5 100644 --- a/addons/web/static/src/views/fields/properties/property_value.js +++ b/addons/web/static/src/views/fields/properties/property_value.js @@ -359,6 +359,7 @@ PropertyValue.props = { context: { type: Object }, readonly: { type: Boolean, optional: true }, canChangeDefinition: { type: Boolean, optional: true }, + checkDefinitionWriteAccess: { type: Function, optional: true }, selection: { type: Array, optional: true }, tags: { type: Array, optional: true }, onChange: { type: Function, optional: true }, diff --git a/addons/web/static/src/views/fields/properties/property_value.xml b/addons/web/static/src/views/fields/properties/property_value.xml index f7ab6bf1ca5..fb3750079f4 100644 --- a/addons/web/static/src/views/fields/properties/property_value.xml +++ b/addons/web/static/src/views/fields/properties/property_value.xml @@ -66,6 +66,7 @@ deleteAction="'value'" readonly="props.readonly" canChangeTags="props.canChangeDefinition" + checkDefinitionWriteAccess.bind="props.checkDefinitionWriteAccess" onValueChange.bind="onValueChange" onTagsChange.bind="props.onTagsChange"/> diff --git a/addons/web/static/tests/views/fields/properties_field_tests.js b/addons/web/static/tests/views/fields/properties_field_tests.js index 2c5eb8065c0..96d5534be3a 100644 --- a/addons/web/static/tests/views/fields/properties_field_tests.js +++ b/addons/web/static/tests/views/fields/properties_field_tests.js @@ -281,7 +281,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: access to parent", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } } @@ -378,7 +378,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: add a new property", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } } @@ -435,7 +435,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: selection", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } } @@ -573,7 +573,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: float and integer", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } } @@ -642,7 +642,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: move properties", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } } @@ -717,7 +717,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: tags", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } } @@ -837,7 +837,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: many2one", async function (assert) { async function mockRPC(route, { method, model, args, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } else if (method === "get_available_models" && model === "ir.model") { return [ @@ -923,7 +923,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: many2many", async function (assert) { async function mockRPC(route, { method, model, args, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } else if (method === "get_available_models" && model === "ir.model") { return [ @@ -1026,7 +1026,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: many2one 'Search more...'", async function (assert) { async function mockRPC(route, { method, model }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } else if (method === "display_name_for" && model === "ir.model") { return [ @@ -1223,7 +1223,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: name reset", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } else if (method === "get_available_models" && model === "ir.model") { return [ @@ -1361,7 +1361,7 @@ QUnit.module("Fields", (hooks) => { */ QUnit.test("properties: default value", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } } @@ -1428,7 +1428,7 @@ QUnit.module("Fields", (hooks) => { "properties: form view and falsy domain, properties are not empty", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } } @@ -1482,7 +1482,7 @@ QUnit.module("Fields", (hooks) => { "properties: form view and falsy domain, properties are empty", async function (assert) { async function mockRPC(route, { method, model, kwargs }) { - if (method === "check_access_rights") { + if (["check_access_rights", "check_access_rule"].includes(method)) { return true; } } diff --git a/odoo/addons/test_new_api/tests/test_properties.py b/odoo/addons/test_new_api/tests/test_properties.py index 5f0986c5b84..85712aeea26 100644 --- a/odoo/addons/test_new_api/tests/test_properties.py +++ b/odoo/addons/test_new_api/tests/test_properties.py @@ -1383,31 +1383,42 @@ class PropertiesCase(TransactionCase): self.assertEqual(expected_properties, sql_properties) @mute_logger('odoo.fields') + @users('test') def test_properties_field_security(self): """Check the access right related to the Properties fields.""" - MultiTag = type(self.env['test_new_api.multi.tag']) - - def _mocked_check_access_rights(operation, raise_exception=True): + def _mocked_check_access_rights(records, operation, raise_exception=True): + if records.env.su: # called with SUDO + return True if raise_exception: raise AccessError('') return False + message = self.message_1.with_user(self.test_user) + # a user read a properties with a many2one to a record he doesn't have access to tag = self.env['test_new_api.multi.tag'].create({'name': 'Test Tag'}) - self.message_1.attributes = [{ + + message.attributes = [{ 'name': 'test', 'type': 'many2one', 'comodel': 'test_new_api.multi.tag', 'value': [tag.id, 'Tag'], 'definition_changed': True, }] - values = self.message_1.read(['attributes'])[0]['attributes'][0] + values = message.read(['attributes'])[0]['attributes'][0] self.assertEqual(values['value'], (tag.id, 'Test Tag')) self.env.invalidate_all() - with patch.object(MultiTag, 'check_access_rights', side_effect=_mocked_check_access_rights): - values = self.message_1.read(['attributes'])[0]['attributes'][0] + with patch('odoo.addons.test_new_api.models.test_new_api.MultiTag.check_access_rights', _mocked_check_access_rights): + values = message.read(['attributes'])[0]['attributes'][0] self.assertEqual(values['value'], (tag.id, None)) + # a user read a properties with a many2one to a record + # but doesn't have access to its parent + self.env.invalidate_all() + with patch('odoo.addons.test_new_api.models.test_new_api.Discussion.check_access_rights', _mocked_check_access_rights): + values = message.read(['attributes'])[0]['attributes'][0] + self.assertEqual(values['value'], (tag.id, 'Test Tag')) + def _get_sql_properties(self, message): self.env.flush_all()