diff --git a/fs_attachment/fs_stream.py b/fs_attachment/fs_stream.py index 28d3ea9690..64908201cc 100644 --- a/fs_attachment/fs_stream.py +++ b/fs_attachment/fs_stream.py @@ -28,6 +28,7 @@ def from_fs_attachment(cls, attachment: IrAttachment) -> FsStream: type="fs", size=attachment.file_size, last_modified=attachment["__last_update"], + public=attachment.public, fs_attachment=attachment, ) @@ -46,7 +47,10 @@ def get_response( ): if self.type != "fs": return super().get_response( - as_attachment=as_attachment, immutable=immutable, **send_file_kwargs + as_attachment=as_attachment, + immutable=immutable, + content_security_policy=content_security_policy, + **send_file_kwargs, ) if as_attachment is None: as_attachment = self.as_attachment @@ -64,6 +68,7 @@ def get_response( "max_age": STATIC_CACHE_LONG if immutable else self.max_age, "environ": request.httprequest.environ, "response_class": Response, + **send_file_kwargs, } use_x_sendfile = self.fs_attachment._fs_use_x_sendfile() # The file will be closed by werkzeug... @@ -81,12 +86,19 @@ def get_response( res.headers["X-Sendfile"] = x_sendfile_path res.headers["Content-Length"] = 0 - if immutable and res.cache_control: - res.cache_control["immutable"] = None - res.headers["X-Content-Type-Options"] = "nosniff" if content_security_policy: res.headers["Content-Security-Policy"] = content_security_policy + # apply same cache policy as odoo.http.Stream.get_response + if self.public: + if (res.cache_control.max_age or 0) > 0: + res.cache_control.public = True + else: + res.cache_control.pop("public", "") + res.cache_control.private = True + if immutable: + res.cache_control["immutable"] = None + return res diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index b75ca583c7..ea24686636 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -9,6 +9,7 @@ import mimetypes import os import re +import shutil import time from contextlib import closing, contextmanager from pathlib import Path @@ -17,7 +18,6 @@ import psycopg2 from slugify import slugify # pylint: disable=missing-manifest-dependency -import odoo from odoo import _, api, fields, models from odoo.exceptions import AccessError, UserError from odoo.osv.expression import AND, OR, normalize_domain @@ -131,7 +131,7 @@ def _compute_fs_url_path(self) -> None: rec, exclude_base_url=True ) - @api.depends("fs_filename") + @api.depends("store_fname", "fs_filename") def _compute_fs_storage_id(self): for rec in self: if rec.store_fname: @@ -414,6 +414,7 @@ def _storage_file_write(self, bin_data: bytes) -> str: with fs.open(path, "wb", **kwargs) as f: f.write(bin_data) self._fs_mark_for_gc(fname) + self._fs_get_files_written_in_transaction().add(fname) return fname @api.model @@ -478,7 +479,7 @@ def _enforce_meaningful_storage_filename(self) -> None: Keeping the same meaning and mimetype is important to also ease to provide a meaningful and SEO friendly URL to the file in the filesystem storage. """ - renamed_attachments = {} + to_rename = self.browse() for attachment in self: if not self._is_file_from_a_storage(attachment.store_fname): continue @@ -486,22 +487,36 @@ def _enforce_meaningful_storage_filename(self) -> None: if self.env["fs.storage"]._must_use_filename_obfuscation(storage): attachment.fs_filename = filename - continue + else: + to_rename |= attachment + to_rename._set_meaningful_storage_filename() + + def _set_meaningful_storage_filename(self) -> None: + """Give each attachment's file its meaningful name""" + usage = self._count_by_store_fname() + written_files = self._fs_get_files_written_in_transaction() + for attachment in self: + fs, storage, filename = attachment._get_fs_parts() + old_store_fname = attachment.store_fname new_filename = attachment._build_fs_filename() # we must keep the same full path as the original filename new_filename_with_path = os.path.join( os.path.dirname(filename), new_filename ) - if filename in renamed_attachments: - if renamed_attachments[filename] == new_filename_with_path: - # we already renamed this file, no need to rename it again - continue - else: - fs.copy(renamed_attachments[filename], new_filename_with_path) + if usage[old_store_fname] > 1: + # the file stays with the other attachments stored in it + usage[old_store_fname] -= 1 + fs.copy(filename, new_filename_with_path) + elif old_store_fname not in written_files: + # The file has not been written by the current transaction + # We can't rename it to ensure that in case of rollback + # the original one still exists... In place we copy + # the original and we mark it to GC + fs.copy(filename, new_filename_with_path) + self._fs_mark_for_gc(old_store_fname) else: fs.rename(filename, new_filename_with_path) - renamed_attachments[filename] = new_filename_with_path attachment.fs_filename = new_filename # we need to update the store_fname with the new filename by @@ -510,6 +525,31 @@ def _enforce_meaningful_storage_filename(self) -> None: # flake8: noqa: E231 attachment._force_write_store_fname(f"{storage}://{new_filename_with_path}") self._fs_mark_for_gc(attachment.store_fname) + written_files.add(attachment.store_fname) + + @api.model + def _fs_get_files_written_in_transaction(self) -> set[str]: + """Return the store_fname of the files written into a filesystem + storage by the current transaction. + """ + return self.env.cr.postcommit.data.setdefault( + "fs_attachment.written_files", set() + ) + + def _count_by_store_fname(self) -> dict[str, int]: + """Return the number of attachments stored in each file of self""" + # "res_field = False OR res_field != False" to count the attachments of + # binary fields too, see fs.storage + domain = [ + ("store_fname", "in", self.mapped("store_fname")), + "|", + ("res_field", "=", False), + ("res_field", "!=", False), + ] + groups = self.sudo()._read_group( + domain, ["store_fname"], ["store_fname"], lazy=False + ) + return {group["store_fname"]: group["__count"] for group in groups} def _force_write_store_fname(self, store_fname): """Force the write of the store_fname field @@ -520,6 +560,16 @@ def _force_write_store_fname(self, store_fname): """ self._fields["store_fname"].write(self, store_fname) + def _force_write_file_values(self, vals): + """Force the write of the fields computed from the file content + + The write method of ir_attachment ignores the fields 'checksum' and + 'file_size'. This method bypass this limitation by calling the write + method of the fields directly. + """ + for field_name, value in vals.items(): + self._fields[field_name].write(self, value) + @api.model def _get_fs_storage_for_code( self, @@ -678,8 +728,7 @@ def _do_in_new_env(self, new_cr=False): Using a new Odoo Environment thus a new PG transaction. """ if new_cr: - registry = odoo.modules.registry.Registry.new(self.env.cr.dbname) - with closing(registry.cursor()) as cr: + with closing(self.env.registry.cursor()) as cr: try: yield self.env(cr=cr) except Exception: @@ -1089,19 +1138,9 @@ def _file_open(self) -> io.IOBase: attachment_res_model=self.attachment.res_model, attachment_res_field=self.attachment.res_field, )._file_write(content, checksum) - if self.attachment._is_file_from_a_storage(new_store_fname): - ( - filesystem, - _storage, - new_filepath, - ) = self.attachment._fs_parse_store_fname(new_store_fname) - _fs, _storage, old_filepath = self.attachment._get_fs_parts() - else: - new_filepath = self.attachment._full_path(new_store_fname) - old_filepath = self.attachment._full_path(self.attachment.store_fname) - filesystem = fsspec.filesystem("file") + filesystem, new_filepath = self._get_filesystem_and_path(new_store_fname) if "a" in self.mode: - filesystem.cp_file(old_filepath, new_filepath) + self._copy_current_content(filesystem, new_filepath, new_store_fname) the_file = filesystem.open( new_filepath, mode=self.mode, @@ -1114,6 +1153,49 @@ def _file_open(self) -> io.IOBase: self._new_store_fname = new_store_fname self._file = the_file + def _get_filesystem_and_path( + self, store_fname: str + ) -> tuple[fsspec.AbstractFileSystem, str]: + """Return the filesystem and the path of the file for the given + store_fname (filesystem storage or odoo filestore)""" + if self.attachment._is_file_from_a_storage(store_fname): + filesystem, _storage, path = self.attachment._fs_parse_store_fname( + store_fname + ) + return filesystem, path + return fsspec.filesystem("file"), self.attachment._full_path(store_fname) + + def _get_location(self, store_fname: str) -> str: + """Return the code of the filesystem storage where the file is + stored or 'file' for the odoo filestore""" + if self.attachment._is_file_from_a_storage(store_fname): + return store_fname.partition("://")[0] + return "file" + + def _copy_current_content( + self, + filesystem: fsspec.AbstractFileSystem, + new_filepath: str, + new_store_fname: str, + ) -> None: + """Copy the current content of the attachment into the new file + + The current content can be stored in the database, in the odoo + filestore or in a filesystem storage different from the one of the + new file. + """ + old_store_fname = self.attachment.store_fname + if old_store_fname and self._get_location( + old_store_fname + ) == self._get_location(new_store_fname): + # same location: copy the file + _filesystem, old_filepath = self._get_filesystem_and_path(old_store_fname) + filesystem.cp_file(old_filepath, new_filepath) + return + with AttachmentFileLikeAdapter(self.attachment, mode="rb") as src: + with filesystem.open(new_filepath, "wb") as dst: + shutil.copyfileobj(src, dst) + def _gen_random_content(self, size=256): """Generate a random content of size bytes""" return os.urandom(size) @@ -1132,7 +1214,15 @@ def _file_close(self): and self._is_stored_in_db ): attachment_data["raw"] = self._file.getvalue() + # the values computed from the content are removed from the + # values by ir.attachment.write + file_values = { + "checksum": attachment_data["checksum"], + "file_size": attachment_data["file_size"], + } self.attachment.write(attachment_data) + if "raw" not in attachment_data: + self.attachment._force_write_file_values(file_values) if self.new_version and self._new_store_fname: self.attachment._force_write_store_fname(self._new_store_fname) self.attachment._enforce_meaningful_storage_filename() diff --git a/fs_attachment/readme/newsfragments/678.bugfix b/fs_attachment/readme/newsfragments/678.bugfix new file mode 100644 index 0000000000..6a5bb2d8b1 --- /dev/null +++ b/fs_attachment/readme/newsfragments/678.bugfix @@ -0,0 +1,6 @@ +Don't move a file that other attachments still use. + +With filename obfuscation off, storing or renaming an attachment renames its +file. Attachments stored while obfuscation was on share one file per content, +so the others lost their content. The module now copies such a file and only +moves it when no other attachment uses it. diff --git a/fs_attachment/tests/test_fs_attachment.py b/fs_attachment/tests/test_fs_attachment.py index f6a57d6904..7db5571e43 100644 --- a/fs_attachment/tests/test_fs_attachment.py +++ b/fs_attachment/tests/test_fs_attachment.py @@ -347,12 +347,15 @@ def test_force_storage_to_db(self): self.env.flush_all() self.assertTrue(attachment.store_fname) self.assertFalse(attachment.db_datas) + self.assertEqual(attachment.fs_storage_id, self.temp_backend) store_fname = attachment.store_fname # we change the rules to force the storage in db for text/plain self.temp_backend.force_db_for_default_attachment_rules = '{"text/plain": 0}' attachment.force_storage_to_db_for_special_fields() self.assertFalse(attachment.store_fname) self.assertEqual(attachment.db_datas, b"content") + self.assertFalse(attachment.fs_storage_id) + self.assertFalse(attachment.fs_storage_code) # we check that the file is marked for GC gc_files = self.gc_file_model.search([]).mapped("store_fname") self.assertIn(store_fname, gc_files) @@ -479,6 +482,122 @@ def test_create_two_attachments_differnt_call(self): self.assertNotEqual(res[0].store_fname, res2[0].store_fname) self.assertEqual(res[0].raw, res2[0].raw) + def test_create_attachment_same_content_as_obfuscated_one(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.use_filename_obfuscation = True + attachment1 = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + self.temp_backend.use_filename_obfuscation = False + attachment2 = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + self.assertNotEqual(attachment1.store_fname, attachment2.store_fname) + self.env.invalidate_all() + self.assertEqual(attachment1.raw, b"content") + self.assertEqual(attachment2.raw, b"content") + + def test_create_attachment_same_content_as_obfuscated_field_one(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.use_filename_obfuscation = True + attachment1 = self.ir_attachment_model.create( + { + "name": "test.txt", + "raw": b"content", + "res_id": self.env.user.partner_id.id, + "res_model": "res.partner", + "res_field": "image_1920", + } + ) + self.temp_backend.use_filename_obfuscation = False + attachment2 = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + self.assertNotEqual(attachment1.store_fname, attachment2.store_fname) + self.env.invalidate_all() + self.assertEqual(attachment1.raw, b"content") + self.assertEqual(attachment2.raw, b"content") + + def test_write_name_file_shared_with_obfuscated_attachment(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.use_filename_obfuscation = True + attachment1, attachment2 = self.ir_attachment_model.create( + [ + {"name": "test.txt", "raw": b"content"}, + {"name": "test.txt", "raw": b"content"}, + ] + ) + self.assertEqual(attachment1.store_fname, attachment2.store_fname) + self.temp_backend.use_filename_obfuscation = False + attachment1.name = "test2.txt" + self.assertNotEqual(attachment1.store_fname, attachment2.store_fname) + self.env.invalidate_all() + self.assertEqual(attachment1.raw, b"content") + self.assertEqual(attachment2.raw, b"content") + + def test_open_new_version_content_stored_elsewhere(self): + """Open a new version of a file into a filesystem storage while the + current content is stored in the database or in the odoo filestore""" + for location in ("db", "file"): + for mode, expected in ( + ("wb", b" new"), + ("ab", b"initial new"), + ): + with self.subTest(location=location, mode=mode): + self.temp_backend.use_as_default_for_attachments = False + self.env["ir.config_parameter"].sudo().set_param( + "ir_attachment.location", location + ) + attachment = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"initial"} + ) + self.assertFalse(attachment.fs_storage_id) + self.temp_backend.use_as_default_for_attachments = True + with attachment.open(mode, new_version=True) as f: + f.write(b" new") + self.assertEqual(attachment.raw, expected) + self.assertTrue(attachment.store_fname.startswith("tmp_dir://")) + + def test_write_name_transactional(self): + """If the transaction is rolled back after a change of name, the + attachment must still reference an existing file""" + self.temp_backend.use_as_default_for_attachments = True + attachment = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + self.env.flush_all() + initial_filename = f"test-{attachment.id}-0.txt" + initial_store_fname = attachment.store_fname + self.assertEqual(initial_store_fname, f"tmp_dir://{initial_filename}") + # simulate the commit of the transaction that created the file + self.env.cr.postcommit.data.pop("fs_attachment.written_files", None) + try: + with self.env.cr.savepoint(): + attachment.name = "test2.txt" + new_filename = f"test2-{attachment.id}-1.txt" + self.assertEqual(attachment.store_fname, f"tmp_dir://{new_filename}") + # the original file is copied, not moved + self.assertEqual( + set(os.listdir(self.temp_dir)), {initial_filename, new_filename} + ) + gc_files = self.gc_file_model.search([]).mapped("store_fname") + self.assertIn(initial_store_fname, gc_files) + raise MyException("dummy exception") + except MyException: + ... + self.assertEqual(attachment.store_fname, initial_store_fname) + self.assertEqual(attachment.raw, b"content") + + def test_write_name_of_new_attachment_renames_file(self): + """A file written by the current transaction is renamed, not copied""" + self.temp_backend.use_as_default_for_attachments = True + attachment = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + attachment.name = "test2.txt" + self.env.flush_all() + self.assertEqual(os.listdir(self.temp_dir), [f"test2-{attachment.id}-1.txt"]) + def test_update_png_to_svg(self): b64_data_png = ( b"iVBORw0KGgoAAAANSUhEUgAAADMAAAAhCAIAAAD73QTtAAAAA3NCSVQICAjb4U/gAA" diff --git a/fs_attachment/tests/test_fs_attachment_file_like_adapter.py b/fs_attachment/tests/test_fs_attachment_file_like_adapter.py index bac729c136..edcbaad580 100644 --- a/fs_attachment/tests/test_fs_attachment_file_like_adapter.py +++ b/fs_attachment/tests/test_fs_attachment_file_like_adapter.py @@ -17,6 +17,7 @@ def prepareClass(cls): def prepare(self): self.attachment = self._create_attachment() + self.initial_checksum = self.attachment.checksum def open(self, attachment=None, mode="rb", new_version=False, **kwargs): return AttachmentFileLikeAdapter( @@ -26,6 +27,13 @@ def open(self, attachment=None, mode="rb", new_version=False, **kwargs): **kwargs, ) + def assertFileValues(self, content): + """The values computed from the content must be up to date""" + # the checksum is used as ETag and must change with the content + self.assertTrue(self.attachment.checksum) + self.assertNotEqual(self.attachment.checksum, self.initial_checksum) + self.assertEqual(self.attachment.file_size, len(content)) + def test_read(self): with self.open(mode="rb") as f: self.assertEqual(f.read(), self.initial_content) @@ -34,18 +42,21 @@ def test_write(self): with self.open(mode="wb") as f: f.write(self.new_content) self.assertEqual(self.new_content, self.attachment.raw) + self.assertFileValues(self.new_content) def test_write_append(self): self.assertEqual(self.initial_content, self.attachment.raw) with self.open(mode="ab") as f: f.write(self.new_content) self.assertEqual(self.initial_content + self.new_content, self.attachment.raw) + self.assertFileValues(self.initial_content + self.new_content) def test_write_new_version(self): initial_fname = self.attachment.store_fname with self.open(mode="wb", new_version=True) as f: f.write(self.new_content) self.assertEqual(self.new_content, self.attachment.raw) + self.assertFileValues(self.new_content) if initial_fname: self.assertNotEqual(self.attachment.store_fname, initial_fname) @@ -54,6 +65,7 @@ def test_write_append_new_version(self): with self.open(mode="ab", new_version=True) as f: f.write(self.new_content) self.assertEqual(self.initial_content + self.new_content, self.attachment.raw) + self.assertFileValues(self.initial_content + self.new_content) if initial_fname: self.assertNotEqual(self.attachment.store_fname, initial_fname) diff --git a/fs_attachment/tests/test_stream.py b/fs_attachment/tests/test_stream.py index 5f5c32288b..ab11e31509 100644 --- a/fs_attachment/tests/test_stream.py +++ b/fs_attachment/tests/test_stream.py @@ -162,6 +162,31 @@ def test_response_csp_header(self): }, ) + def test_response_cache_control_private(self): + self.authenticate("admin", "admin") + for url in ( + f"/web/content/{self.attachment_binary.id}", + # immutable content must not become cacheable by proxies + f"/web/content/{self.attachment_binary.id}?unique=1", + ): + res = self.assertDownload( + url, headers={}, assert_status_code=200, assert_headers={} + ) + cache_control = res.headers.get("Cache-Control", "") + self.assertIn("private", cache_control) + self.assertNotIn("public", cache_control) + + def test_response_cache_control_public(self): + self.attachment_binary.public = True + url = f"/web/content/{self.attachment_binary.id}?unique=1" + res = self.assertDownload( + url, headers={}, assert_status_code=200, assert_headers={} + ) + cache_control = res.headers.get("Cache-Control", "") + self.assertIn("public", cache_control) + self.assertIn("immutable", cache_control) + self.assertNotIn("private", cache_control) + def test_serving_field_image(self): self.authenticate("admin", "admin") demo_partner = self.env.ref("base.partner_demo") diff --git a/fs_image/tests/test_fs_image.py b/fs_image/tests/test_fs_image.py index fb9a9b8f64..1520f53ed9 100644 --- a/fs_image/tests/test_fs_image.py +++ b/fs_image/tests/test_fs_image.py @@ -228,7 +228,7 @@ def test_generated_sql_commands(self): # if the number of SQL queries changes into the logs. They # are to help us keep track of the number of SQL queries generated # by the module. - with self.assertQueryCount(__system__=3): + with self.assertQueryCount(__system__=4): instance = self.env["test.image.model"].create( {"fs_image": FSImageValue(name=self.filename, value=self.image_w)} )