[FIX] base, web: allow reading property fields when no access to parent record

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) <awa@odoo.com>

addons/web/i18n/web.pot

@@ -5972,6 +5972,21 @@ msgid ""
 "You may not believe it,<br />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

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)
             ),

addons/web/static/src/views/fields/properties/properties_field.xml

@@ -31,6 +31,7 @@
                             <PropertyValue
                                 id="domId"
                                 canChangeDefinition="state.canChangeDefinition"
+                                checkDefinitionWriteAccess.bind="checkDefinitionWriteAccess"
                                 comodel="propertyConfiguration.comodel || ''"
                                 context="props.context"
                                 domain="propertyConfiguration.domain || '[]'"

addons/web/static/src/views/fields/properties/property_definition.js

@@ -399,6 +399,7 @@ PropertyDefinition.components = {
 PropertyDefinition.props = {
     readonly: { type: Boolean, optional: true },
     canChangeDefinition: { type: Boolean, optional: true },
+    checkDefinitionWriteAccess: { type: Function, optional: true },
     propertyDefinition: { optional: true },
     hideKanbanOption: { type: Boolean, optional: true },
     context: { type: Object },

addons/web/static/src/views/fields/properties/property_definition.xml

@@ -102,6 +102,7 @@
                             tags="state.propertyDefinition.tags || []"
                             readonly="props.readonly"
                             canChangeTags="props.canChangeDefinition"
+                            checkDefinitionWriteAccess.bind="props.checkDefinitionWriteAccess"
                             deleteAction="'tags'"
                             onTagsChange.bind="onTagsChange"/>
                     </td>

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

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 },

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"/>
             </t>

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;
                 }
             }

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()