From 171bd86a1b59c846fc41eb47d2bcdbe739363111 Mon Sep 17 00:00:00 2001 From: Don Kendall Date: Mon, 22 Jun 2026 20:29:27 -0400 Subject: [PATCH 1/2] [IMP] dms: method-based record access, drop computed-permission ir.rules --- dms/models/dms_security_mixin.py | 142 +++++++++++++++---------------- dms/security/security.xml | 81 ------------------ 2 files changed, 67 insertions(+), 156 deletions(-) diff --git a/dms/models/dms_security_mixin.py b/dms/models/dms_security_mixin.py index b671ab820..a76d7038f 100644 --- a/dms/models/dms_security_mixin.py +++ b/dms/models/dms_security_mixin.py @@ -4,9 +4,10 @@ # License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). +import functools from logging import getLogger -from odoo import SUPERUSER_ID, api, fields, models +from odoo import api, fields, models from odoo.exceptions import AccessError from odoo.fields import Domain from odoo.tools import SQL @@ -197,34 +198,25 @@ def _get_domain_by_access_groups(self, operation): return result @api.model - def _get_permission_domain(self, operator, value, operation): - """Abstract logic for searching computed permission fields.""" - _self = self - # HACK ir.rule domains are evaluated in superuser mode while env.uid - # stays the acting user, so `su` together with a non-root uid means we - # are resolving the `permission_ = user.id` rule on that user's - # behalf. The Domain engine coerces that sentinel to this Boolean - # field's type before we get here, so we rely on env.uid (used by - # _get_access_groups_query) rather than the value to build the domain. - if self.env.su and self.env.uid != SUPERUSER_ID: - _self = self.sudo(False) - value = bool(value) - # Tricky one, to know if you want to search - # positive or negative access - positive = (operator not in Domain.NEGATIVE_OPERATORS) == bool(value) - if _self.env.su: - # You're SUPERUSER_ID - return Domain.TRUE if positive else Domain.FALSE - - result = Domain.OR( + def _get_dms_access_domain(self, operation): + """Domain matching the records the current user may access for + ``operation`` through DMS access groups or inheritance.""" + return Domain.OR( [ - _self._get_domain_by_access_groups(operation), - _self._get_domain_by_inheritance(operation), + self._get_domain_by_access_groups(operation), + self._get_domain_by_inheritance(operation), ] ) - if not positive: - result = ~Domain(result) - return result + + @api.model + def _get_permission_domain(self, operator, value, operation): + """Search implementation of the computed ``permission_`` fields, + used by field domains (e.g. ``directory_id``'s create filter).""" + positive = (operator not in Domain.NEGATIVE_OPERATORS) == bool(value) + if self.env.su: + return Domain.TRUE if positive else Domain.FALSE + result = self._get_dms_access_domain(operation) + return result if positive else ~result @api.model def _search_permission_create(self, operator, value): @@ -242,49 +234,57 @@ def _search_permission_unlink(self, operator, value): def _search_permission_write(self, operator, value): return self._get_permission_domain(operator, value, "write") - def filtered_domain(self, domain): - """This method is needed to inhibit the behavior when called from the - _check_access() method with sudo() https://github.com/odoo/odoo/blob/fc737a147b9aefbd6ae5d111835ce3f4f7b4240a/odoo/models.py#L4465. - It would cause the error that multiple records are not accessed to be - displayed. - The _filtered_access() method is also overwritten to prevent this sudo() - specific behavior and to be able to access only the appropriate records. - """ - if self.env.su: - return self - return super().filtered_domain(domain) - - def _filtered_access_no_recursion(self, operation: str): - """This method is just the same as _filtered_access - but it can not be called withoud super due to - recursion error. - """ - if self and not self.env.su and (result := self._check_access(operation)): - return self - result[0] - return self + def _search( + self, + domain, + offset=0, + limit=None, + order=None, + *, + bypass_access=False, + **kwargs, + ): + """Restrict searches to the records the current user may read through + DMS access groups or inheritance (mirrors ``mail.message._search``).""" + if self.env.su or bypass_access: + return super()._search( + domain, offset, limit, order, bypass_access=bypass_access, **kwargs + ) + domain = Domain.AND([Domain(domain), self._get_dms_access_domain("read")]) + return super()._search(domain, offset, limit, order, **kwargs) - def _filtered_access(self, operation): - # Only kept to not break inheritance; see next comment - result = super()._filtered_access(operation) - # HACK Always fall back to applying rules by SQL. - # Upstream `_filtered_access()` doesn't use computed fields - # search methods. Thus, it will take the `[('permission_{operation}', - # '=', user.id)]` rule literally. Obviously that will always fail - # because `self[f"permission_{operation}"]` will always be a `bool`, - # while `user.id` will always be an `int`. - result |= self._filtered_access_no_recursion(operation) + def _check_access(self, operation): + """Add the DMS access-group / inheritance restriction to the + record-level access check (mirrors ``mail.message._check_access``).""" + result = super()._check_access(operation) + if self.env.su or not any(self._ids): + return result + records = self - result[0] if result else self + forbidden = records._get_forbidden_dms_access(operation) + if forbidden: + if result: + return result[0] + forbidden, result[1] + Rule = self.env["ir.rule"] + return forbidden, functools.partial( + Rule._make_access_error, operation, forbidden + ) return result - def _check_access_dms_record(self, operation: str) -> tuple | None: - """Specific method "similar" to _check_access() but with a different - behavior: check if you do not really have access to any of the records - in to avoid performing the corresponding create/write/unlink action.""" - if any(self._ids) and not self.env.su: - Rule = self.env["ir.rule"] - domain = Rule._compute_domain(self._name, operation) - items = self.with_context(active_test=False).search(domain) - if any(x_id not in items.ids for x_id in self.ids): - raise Rule._make_access_error(operation, (self - items)) + def _get_forbidden_dms_access(self, operation): + """Return the subset of ``self`` the current user cannot access for + ``operation`` under the DMS access-group / inheritance rules. The + domain carries an SQL sub-query, so it is resolved by search (not by + ``filtered_domain``), bypassing access to evaluate exactly this domain.""" + domain = Domain.AND( + [ + Domain([("id", "in", self.ids)]), + self._get_dms_access_domain(operation), + ] + ) + allowed = self.browse( + self.with_context(active_test=False)._search(domain, bypass_access=True) + ) + return self - allowed @api.model_create_multi def create(self, vals_list): @@ -296,13 +296,5 @@ def create(self, vals_list): res.flush_recordset() # Go back to the original sudo state and check we really had creation permission res = res.sudo(self.env.su) - res._check_access_dms_record("create") + res.check_access("create") return res - - def write(self, vals): - self._check_access_dms_record("write") - return super().write(vals) - - def unlink(self): - self._check_access_dms_record("unlink") - return super().unlink() diff --git a/dms/security/security.xml b/dms/security/security.xml index 5f6bf0db8..387d32e7b 100644 --- a/dms/security/security.xml +++ b/dms/security/security.xml @@ -111,85 +111,4 @@ [('is_hidden', '=', True)] - - - Apply computed create permissions. - - - - - - - [('permission_create', '=', user.id)] - - - Apply computed read permissions. - - - - - - - [('permission_read', '=', user.id)] - - - Apply computed unlink permissions. - - - - - - - [('permission_unlink', '=', user.id)] - - - Apply computed write permissions. - - - - - - - [('permission_write', '=', user.id)] - - - Apply computed create permissions. - - - - - - - [('permission_create', '=', user.id)] - - - Apply computed read permissions. - - - - - - - [('permission_read', '=', user.id)] - - - Apply computed unlink permissions. - - - - - - - [('permission_unlink', '=', user.id)] - - - Apply computed write permissions. - - - - - - - [('permission_write', '=', user.id)] - From 7fcc99b80bfa77bce58aee403c915576e9a93215 Mon Sep 17 00:00:00 2001 From: Erik Papernyuk <80971885+Erp4759@users.noreply.github.com> Date: Tue, 7 Jul 2026 14:16:03 -0400 Subject: [PATCH 2/2] [FIX] dms: restore contextual-creation defaults on 19.0, date-only kanban MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 19.0 web client sanitizes default_* and searchpanel_default_* out of the context it sends when creating a record from a contextual child view (smart button -> New); only active_model/active_id survive. parent_id is computed-writable, so the value resolves in the compute, where the old self-assign no longer preserves anything on new records. Resolve the parent/directory from the surviving active_model/active_id keys — the same contract _default_parent_id already used — in the compute and the file's directory_id default. Also: date-only rendering of write_date via the datetime option (show_time: false) instead of widget="date", and regression tests for contextual defaults, read-only access groups, restricted binary URLs and multipart upload/download. Co-authored-by: Don Kendall --- dms/models/directory.py | 13 +++++++++-- dms/models/dms_file.py | 8 +++++++ dms/tests/test_directory.py | 17 +++++++++++++- dms/tests/test_file.py | 45 +++++++++++++++++++++++++++++++++++++ dms/tests/test_portal.py | 43 +++++++++++++++++++++++++++++++++++ dms/views/dms_directory.xml | 5 ++++- dms/views/dms_file.xml | 5 ++++- 7 files changed, 131 insertions(+), 5 deletions(-) diff --git a/dms/models/directory.py b/dms/models/directory.py index 33dc931ea..89d122711 100644 --- a/dms/models/directory.py +++ b/dms/models/directory.py @@ -490,8 +490,17 @@ def _compute_parent_id(self): if record.is_root_directory: record.parent_id = None else: - # HACK: Not needed in v14 due to odoo/odoo#64359 - record.parent_id = record.parent_id + ctx = self.env.context + record.parent_id = ( + record.parent_id + or record._origin.parent_id + or ctx.get("default_parent_id") + or ( + ctx.get("active_id") + if ctx.get("active_model") == "dms.directory" + else False + ) + ) @api.depends("is_root_directory", "parent_id") def _compute_root_id(self): diff --git a/dms/models/dms_file.py b/dms/models/dms_file.py index 544201498..6c8bd3168 100644 --- a/dms/models/dms_file.py +++ b/dms/models/dms_file.py @@ -54,6 +54,14 @@ class DMSFile(models.Model): required=True, index="btree", tracking=True, # Leave log if "moved" to another directory + default=lambda self: ( + self.env.context.get("default_directory_id") + or ( + self.env.context.get("active_id") + if self.env.context.get("active_model") == "dms.directory" + else False + ) + ), ) root_directory_id = fields.Many2one(related="directory_id.root_directory_id") # Override acording to defined in AbstractDmsMixin diff --git a/dms/tests/test_directory.py b/dms/tests/test_directory.py index 3f69b5df7..bdbc80172 100644 --- a/dms/tests/test_directory.py +++ b/dms/tests/test_directory.py @@ -8,7 +8,7 @@ from odoo import Command from odoo.exceptions import AccessError, UserError -from odoo.tests import new_test_user +from odoo.tests import Form, new_test_user from odoo.tests.common import users from odoo.tools import mute_logger @@ -41,6 +41,21 @@ def test_create_directory(self): msg="The root directory should have one subdirectory", ) + def test_default_parent_from_context(self): + """Creating from a directory's contextual views: the 19.0 web client + sanitizes default_* (and searchpanel_default_*) out of the new-record + context — only active_model/active_id survive. The parent/directory + default must resolve from those.""" + ctx = { + "active_model": "dms.directory", + "active_id": self.directory.id, + "active_ids": [self.directory.id], + } + directory_form = Form(self.directory_model.with_context(**ctx)) + self.assertEqual(directory_form.parent_id, self.directory) + file_form = Form(self.file_model.with_context(**ctx)) + self.assertEqual(file_form.directory_id, self.directory) + @users("dms-manager", "dms-user") def test_copy_root_directory(self): copy_root_directory = self.directory.copy() diff --git a/dms/tests/test_file.py b/dms/tests/test_file.py index e91526271..bb2cae185 100644 --- a/dms/tests/test_file.py +++ b/dms/tests/test_file.py @@ -47,6 +47,21 @@ def setUpClass(cls): ) cls.directory_group_a.group_ids = [(4, cls.group_a.id)] cls.file2 = cls.create_file(directory=cls.sub_directory_x) + cls.readonly_user = new_test_user( + cls.env, login="read-only", groups="dms.group_dms_user" + ) + cls.readonly_group = cls.access_group_model.create( + { + "name": "Read only", + "explicit_user_ids": [(6, 0, [cls.readonly_user.id])], + } + ) + cls.readonly_directory = cls.create_directory(storage=cls.storage) + cls.readonly_directory.group_ids = [(6, 0, cls.readonly_group.ids)] + cls.readonly_subdirectory = cls.create_directory( + directory=cls.readonly_directory + ) + cls.readonly_file = cls.create_file(directory=cls.readonly_subdirectory) @users("user-a") def test_unaccessible_file(self): @@ -140,6 +155,36 @@ def test_record_level_access(self): } ) + @users("read-only") + @mute_logger("odoo.addons.base.models.ir_rule", "odoo.models") + def test_read_only_access(self): + """Read-only access groups must not grant mutation permissions.""" + readonly_file = self.readonly_file.with_user(self.env.user) + readonly_file.check_access("read") + for operation in ("write", "unlink"): + with self.assertRaises( + AccessError, msg=f"read-only user {operation} must be denied" + ): + readonly_file.check_access(operation) + with self.assertRaises(AccessError, msg="read-only file write must fail"): + readonly_file.write({"name": "forbidden.txt"}) + with self.assertRaises(AccessError, msg="read-only file create must fail"): + self.file_model.with_user(self.env.user).create( + { + "name": "forbidden.txt", + "directory_id": self.readonly_subdirectory.id, + "content": self.content_base64(), + } + ) + with self.assertRaises(AccessError, msg="read-only directory create must fail"): + self.directory_model.with_user(self.env.user).create( + { + "name": "Forbidden child", + "is_root_directory": False, + "parent_id": self.readonly_directory.id, + } + ) + @users("dms-manager", "dms-user") @mute_logger("odoo.models.unlink") def test_content_file(self): diff --git a/dms/tests/test_portal.py b/dms/tests/test_portal.py index ee6841b4d..12f397924 100644 --- a/dms/tests/test_portal.py +++ b/dms/tests/test_portal.py @@ -1,7 +1,10 @@ # Copyright 2021-2025 Tecnativa - Víctor Martínez # License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl) +import json + import odoo.tests +from odoo import http from odoo.exceptions import AccessError from odoo.tests.common import new_test_user, users from odoo.tools import mute_logger @@ -86,6 +89,46 @@ def test_access_portal(self): self.assertEqual( response.status_code, 200, "Can access directory with correct access_token" ) + # A direct binary URL must not bypass DMS access checks. + response = self.url_open( + f"/web/content?id={self.other_file_partner.id}&field=content" + "&model=dms.file&filename_field=name&download=true", + timeout=20, + ) + self.assertNotEqual( + response.status_code, + 200, + "Can't download a restricted file through a direct URL", + ) + + def test_upload_and_download(self): + upload_directory = self.create_directory(storage=self.create_storage()) + self.authenticate("dms-manager", "dms-manager") + response = self.url_open( + "/web/binary/upload_dms_file", + data={ + "csrf_token": http.Request.csrf_token(self), + "directory_id": upload_directory.id, + }, + files={ + "ufile": ( + "uploaded.txt", + b"Odoo 19 DMS upload", + "text/plain", + ) + }, + ) + response.raise_for_status() + result = json.loads(response.content) + self.assertFalse(result[0].get("error")) + dms_file = self.file_model.browse(result[0]["id"]) + self.assertEqual(dms_file.name, "uploaded.txt") + download = self.url_open( + f"/web/content?id={dms_file.id}&field=content&model=dms.file" + "&filename_field=name&download=true" + ) + download.raise_for_status() + self.assertEqual(download.content, b"Odoo 19 DMS upload") def test_tour(self): for tour in ("dms_portal_mail_tour", "dms_portal_partners_tour"): diff --git a/dms/views/dms_directory.xml b/dms/views/dms_directory.xml index 4d048a53f..ddde91de1 100644 --- a/dms/views/dms_directory.xml +++ b/dms/views/dms_directory.xml @@ -306,7 +306,10 @@ widget="many2many_tags" options="{'color_field': 'color'}" /> - +