From ed1236275ffae625716af5e0dca730c0c26117cd Mon Sep 17 00:00:00 2001
From: fw-bot <otis@odoo.com>
Date: Wed, 16 Oct 2019 12:30:23 +0000
Subject: [PATCH] [FIX] base: Remove label from views if user groups forbid the
 field

Without this patch, if a view defines a `<label for="forbidden_field"/>` anywhere, the web client tries to fetch the field data, but since the user has no access, it gets an error.

Now, if the label targets a forbidden field, it gets removed from the view, as happens with the related field. The client doesn't know of that field's existence and no errors happen.

closes odoo/odoo#38876

Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
---
 odoo/addons/base/models/ir_ui_view.py | 11 ++++++++---
 odoo/addons/base/tests/test_acl.py    | 26 +++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/odoo/addons/base/models/ir_ui_view.py b/odoo/addons/base/models/ir_ui_view.py
index 613ed6e0be1..82e5d34dbbd 100644
--- a/odoo/addons/base/models/ir_ui_view.py
+++ b/odoo/addons/base/models/ir_ui_view.py
@@ -728,11 +728,16 @@ actual arch.
         """
         Model = self.env[model]
 
-        if node.tag == 'field' and node.get('name') in Model._fields:
-            field = Model._fields[node.get('name')]
+        field_name = None
+        if node.tag == "field":
+            field_name = node.get("name")
+        elif node.tag == "label":
+            field_name = node.get("for")
+        if field_name and field_name in Model._fields:
+            field = Model._fields[field_name]
             if field.groups and not self.user_has_groups(groups=field.groups):
                 node.getparent().remove(node)
-                fields.pop(node.get('name'), None)
+                fields.pop(field_name, None)
                 # no point processing view-level ``groups`` anymore, return
                 return False
         if node.get('groups'):
diff --git a/odoo/addons/base/tests/test_acl.py b/odoo/addons/base/tests/test_acl.py
index efecd0c201f..1648cf1dcea 100644
--- a/odoo/addons/base/tests/test_acl.py
+++ b/odoo/addons/base/tests/test_acl.py
@@ -28,6 +28,24 @@ class TestACL(TransactionCase):
            field for users who do not belong to one of the explicitly allowed groups"""
         currency = self.env['res.currency'].with_user(self.demo_user)
 
+        # Add a view that adds a label for the field we are going to check
+        extension = self.env["ir.ui.view"].create({
+            "name": "Add separate label for decimal_places",
+            "model": "res.currency",
+            "inherit_id": self.env.ref("base.view_currency_form").id,
+            "arch": """
+                <data>
+                    <field name="decimal_places" position="attributes">
+                        <attribute name="nolabel">1</attribute>
+                    </field>
+                    <field name="decimal_places" position="before">
+                        <label for="decimal_places"/>
+                    </field>
+                </data>
+            """,
+        })
+        currency = currency.with_context(check_view_ids=extension.ids)
+
         # Verify the test environment first
         original_fields = currency.fields_get([])
         form_view = currency.fields_view_get(False, 'form')
@@ -35,8 +53,10 @@ class TestACL(TransactionCase):
         has_group_system = self.demo_user.has_group(GROUP_SYSTEM)
         self.assertFalse(has_group_system, "`demo` user should not belong to the restricted group before the test")
         self.assertIn('decimal_places', original_fields, "'decimal_places' field must be properly visible before the test")
-        self.assertNotEqual(view_arch.xpath("//field[@name='decimal_places']"), [],
+        self.assertNotEqual(view_arch.xpath("//field[@name='decimal_places'][@nolabel='1']"), [],
                              "Field 'decimal_places' must be found in view definition before the test")
+        self.assertNotEqual(view_arch.xpath("//label[@for='decimal_places']"), [],
+                             "Label for 'decimal_places' must be found in view definition before the test")
 
         # restrict access to the field and check it's gone
         self._set_field_groups(currency, 'decimal_places', GROUP_SYSTEM)
@@ -47,6 +67,8 @@ class TestACL(TransactionCase):
         self.assertNotIn('decimal_places', fields, "'decimal_places' field should be gone")
         self.assertEqual(view_arch.xpath("//field[@name='decimal_places']"), [],
                           "Field 'decimal_places' must not be found in view definition")
+        self.assertEqual(view_arch.xpath("//label[@for='decimal_places']"), [],
+                          "Label for 'decimal_places' must not be found in view definition")
 
         # Make demo user a member of the restricted group and check that the field is back
         self.erp_system_group.users += self.demo_user
@@ -58,6 +80,8 @@ class TestACL(TransactionCase):
         self.assertIn('decimal_places', fields, "'decimal_places' field must be properly visible again")
         self.assertNotEqual(view_arch.xpath("//field[@name='decimal_places']"), [],
                              "Field 'decimal_places' must be found in view definition again")
+        self.assertNotEqual(view_arch.xpath("//label[@for='decimal_places']"), [],
+                             "Label for 'decimal_places' must be found in view definition again")
 
     @mute_logger('odoo.models')
     def test_field_crud_restriction(self):