[FIX] base: attachment copy ACL

Currently, `Attachment.copy` has an *explicit* check for write access
on the underlying record.

This doesn't necessarily make sense e.g. a user copying an attachment
from a template to an email may not have write access to the template,
but that should not be an issue. And indeed if the copy is performed
"by hand" (read then create) things work fine[^1].

Since both `read` and `create` are checked, the explicit `copy` check
doesn't seem necessary. Drop it, and try to add some tests around
`copy`. Move `test_06_linked_record_permission` to its own `TestCase`
and split it for readability.

A secondary issue is that the `check` in `create` was performed in the
context of the `self` being copied, leading to the same issue as
`copy` being repeated (that is, `a.copy()` would call `a.create()`
which would call `a.check('write')`, even though we're semantically
creating a record from scratch). The behavior is really intended for
`write` where we want to check if we have `write` access to both the
"source" and the "destination" records.

Explicitly opt-out of having any source data in the `create` before
performing the `check` calls, this way we correctly and only check for
the writability of the destination, and only check for the readability
of the source.

issue 2746483

[^1] well not entirely true, see 4th paragraph

X-original-commit: 60723fb311ed7cc1cf901fe90e8745835cf4d54d
Part-of: odoo/odoo#85271

addons/mail/wizard/mail_compose_message.py

@@ -263,7 +263,7 @@ class MailComposer(models.TransientModel):
                 new_attachment_ids = []
                 for attachment in wizard.attachment_ids:
                     if attachment in wizard.template_id.attachment_ids:
-                        new_attachment_ids.append(attachment.sudo().copy({'res_model': 'mail.compose.message', 'res_id': wizard.id}).id)
+                        new_attachment_ids.append(attachment.copy({'res_model': 'mail.compose.message', 'res_id': wizard.id}).id)
                     else:
                         new_attachment_ids.append(attachment.id)
                 new_attachment_ids.reverse()

odoo/addons/base/models/ir_attachment.py

@@ -575,7 +575,6 @@ class IrAttachment(models.Model):
         return super(IrAttachment, self).write(vals)
 
     def copy(self, default=None):
-        self.check('write')
         if not (default or {}).keys() & {'datas', 'db_datas', 'raw'}:
             # ensure the content is kept and recomputes checksum/store_fname
             default = dict(default or {}, raw=self.raw)
@@ -626,9 +625,11 @@ class IrAttachment(models.Model):
             # creating multiple attachments on a single record.
             record_tuple = (values.get('res_model'), values.get('res_id'))
             record_tuple_set.add(record_tuple)
-        for record_tuple in record_tuple_set:
-            (res_model, res_id) = record_tuple
-            self.check('create', values={'res_model':res_model, 'res_id':res_id})
+
+        # don't use possible contextual recordset for check, see commit for details
+        Attachments = self.browse()
+        for res_model, res_id in record_tuple_set:
+            Attachments.check('create', values={'res_model':res_model, 'res_id':res_id})
         return super().create(vals_list)
 
     def _post_add_create(self):

odoo/addons/base/tests/test_ir_attachment.py

@@ -85,41 +85,6 @@ class TestIrAttachment(TransactionCase):
         a2_fn = os.path.join(self.filestore, a2_store_fname2)
         self.assertTrue(os.path.isfile(a2_fn))
 
-    def test_06_linked_record_permission(self):
-        Attachment = self.Attachment.with_user(self.env.ref('base.user_demo').id)
-        a1 = self.Attachment.create({'name': 'a1'})
-        vals = {'name': 'attach', 'res_id': a1.id, 'res_model': 'ir.attachment'}
-        a2 = Attachment.create(vals)
-
-        # remove access to linked record a1
-        rule = self.env['ir.rule'].create({
-            'name': 'test_rule', 'domain_force': "[('id', '!=', %s)]" % a1.id,
-            'model_id': self.env.ref('base.model_ir_attachment').id,
-        })
-        a2.invalidate_cache(ids=a2.ids)
-
-        # no read permission on linked record
-        with self.assertRaises(AccessError):
-            a2.datas
-
-        # read permission on linked record
-        rule.perm_read = False
-        a2.datas
-
-        # no write permission on linked record
-        with self.assertRaises(AccessError):
-            a3 = Attachment.create(vals)
-        with self.assertRaises(AccessError):
-            a2.write({'raw': self.blob2})
-        with self.assertRaises(AccessError):
-            a2.unlink()
-
-        # write permission on linked record
-        rule.perm_write = False
-        a4 = Attachment.create(vals)
-        a4.write({'raw': self.blob2})
-        a4.unlink()
-
     def test_07_write_mimetype(self):
         """
         Tests the consistency of documents' mimetypes
@@ -259,3 +224,89 @@ class TestIrAttachment(TransactionCase):
         self.assertEqual(document3.db_datas, False)
         self.assertEqual(document3.store_fname, self.blob1_fname)
         self.assertEqual(document3.checksum, self.blob1_hash)
+
+
+class TestPermissions(TransactionCase):
+    def setUp(self):
+        super().setUp()
+        # replace self.env(uid=1) with an actual user environment so rules apply
+        self.env = self.env(user=self.env.ref('base.user_demo'))
+        self.Attachments = self.env['ir.attachment']
+
+        # create a record with an attachment and a rule allowing Read access
+        # but preventing Create, Update, or Delete
+        record = self.Attachments.create({'name': 'record1'})
+        self.vals = {'name': 'attach', 'res_id': record.id, 'res_model': record._name}
+        a = self.attachment = self.Attachments.create(self.vals)
+
+        # prevent create, write and unlink accesses on record
+        self.rule = self.env['ir.rule'].sudo().create({
+            'name': 'remove access to record %d' % record.id,
+            'model_id': self.env['ir.model']._get_id(record._name),
+            'domain_force': "[('id', '!=', %s)]" % record.id,
+            'perm_read': False
+        })
+        a.flush()
+        a.invalidate_cache(ids=a.ids)
+
+    def test_no_read_permission(self):
+        """If the record can't be read, the attachment can't be read either
+        """
+        # check that the information can be read out of the box
+        self.attachment.datas
+        # prevent read access on record
+        self.rule.perm_read = True
+        self.attachment.invalidate_cache(ids=self.attachment.ids)
+        with self.assertRaises(AccessError):
+            self.attachment.datas
+
+    def test_with_write_permissions(self):
+        """With write permissions to the linked record, attachment can be
+        created, updated, or deleted (or copied).
+        """
+        # enable write permission on linked record
+        self.rule.perm_write = False
+        attachment = self.Attachments.create(self.vals)
+        attachment.copy()
+        attachment.write({'raw': b'test'})
+        attachment.unlink()
+
+    def test_basic_modifications(self):
+        """Lacking write access to the linked record means create, update, and
+        delete on the attachment are forbidden
+        """
+        with self.assertRaises(AccessError):
+            self.Attachments.create(self.vals)
+        with self.assertRaises(AccessError):
+            self.attachment.write({'raw': b'yay'})
+        with self.assertRaises(AccessError):
+            self.attachment.unlink()
+        with self.assertRaises(AccessError):
+            self.attachment.copy()
+
+    def test_cross_record_copies(self):
+        """Copying attachments between records (in the same model or not) adds
+        wrinkles as the ACLs may diverge a lot more
+        """
+        # create an other unwritable record in a different model
+        unwritable = self.env['res.users.log'].create({})
+        with self.assertRaises(AccessError):
+            unwritable.write({})  # checks unwritability
+        # create a writable record in the same model
+        writable = self.Attachments.create({'name': 'yes'})
+        writable.name = 'canwrite'  # checks for writeability
+
+        # can copy from a record with read permissions to one with write permissions
+        copied = self.attachment.copy({'res_model': writable._name, 'res_id': writable.id})
+        # can copy to self given write permission
+        copied.copy()
+        # can not copy back to record without write permission
+        with self.assertRaises(AccessError):
+            copied.copy({'res_id': self.vals['res_id']})
+
+        # can not copy to a record without write permission
+        with self.assertRaises(AccessError):
+            self.attachment.copy({'res_model': unwritable._name, 'res_id': unwritable.id})
+        # even from a record with write permissions
+        with self.assertRaises(AccessError):
+            copied.copy({'res_model': unwritable._name, 'res_id': unwritable.id})