Skip to content

[REVIEW] dms: contextual-creation defaults via active_* contract (lab-tested vs #29) #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dnplkndll wants to merge 2 commits into
base: 19.0-mig-dms
Choose a base branch
from
Open

[REVIEW] dms: contextual-creation defaults via active_* contract (lab-tested vs #29) #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
171bd86
[IMP] dms: method-based record access, drop computed-permission ir.rules
dnplkndll Jun 23, 2026
7fcc99b
[FIX] dms: restore contextual-creation defaults on 19.0, date-only ka…
paperniuk Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions dms/models/directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
8 changes: 8 additions & 0 deletions dms/models/dms_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
142 changes: 67 additions & 75 deletions dms/models/dms_security_mixin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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_<op> = 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_<op>`` 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):
Expand All @@ -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):
Expand All @@ -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()
81 changes: 0 additions & 81 deletions dms/security/security.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,85 +111,4 @@
<field name="perm_unlink" eval="1" />
<field name="domain_force">[('is_hidden', '=', True)]</field>
</record>
<!-- These rules leverage computed permission management -->
<record id="rule_directory_computed_create" model="ir.rule">
<field name="name">Apply computed create permissions.</field>
<field name="model_id" ref="model_dms_directory" />
<field name="global" eval="True" />
<field name="perm_read" eval="0" />
<field name="perm_create" eval="1" />
<field name="perm_write" eval="0" />
<field name="perm_unlink" eval="0" />
<field name="domain_force">[('permission_create', '=', user.id)]</field>
</record>
<record id="rule_directory_computed_read" model="ir.rule">
<field name="name">Apply computed read permissions.</field>
<field name="model_id" ref="model_dms_directory" />
<field name="global" eval="True" />
<field name="perm_read" eval="1" />
<field name="perm_create" eval="0" />
<field name="perm_write" eval="0" />
<field name="perm_unlink" eval="0" />
<field name="domain_force">[('permission_read', '=', user.id)]</field>
</record>
<record id="rule_directory_computed_unlink" model="ir.rule">
<field name="name">Apply computed unlink permissions.</field>
<field name="model_id" ref="model_dms_directory" />
<field name="global" eval="True" />
<field name="perm_read" eval="0" />
<field name="perm_create" eval="0" />
<field name="perm_write" eval="0" />
<field name="perm_unlink" eval="1" />
<field name="domain_force">[('permission_unlink', '=', user.id)]</field>
</record>
<record id="rule_directory_computed_write" model="ir.rule">
<field name="name">Apply computed write permissions.</field>
<field name="model_id" ref="model_dms_directory" />
<field name="global" eval="True" />
<field name="perm_read" eval="0" />
<field name="perm_create" eval="0" />
<field name="perm_write" eval="1" />
<field name="perm_unlink" eval="0" />
<field name="domain_force">[('permission_write', '=', user.id)]</field>
</record>
<record id="rule_file_computed_create" model="ir.rule">
<field name="name">Apply computed create permissions.</field>
<field name="model_id" ref="model_dms_file" />
<field name="global" eval="True" />
<field name="perm_read" eval="0" />
<field name="perm_create" eval="1" />
<field name="perm_write" eval="0" />
<field name="perm_unlink" eval="0" />
<field name="domain_force">[('permission_create', '=', user.id)]</field>
</record>
<record id="rule_file_computed_read" model="ir.rule">
<field name="name">Apply computed read permissions.</field>
<field name="model_id" ref="model_dms_file" />
<field name="global" eval="True" />
<field name="perm_read" eval="1" />
<field name="perm_create" eval="0" />
<field name="perm_write" eval="0" />
<field name="perm_unlink" eval="0" />
<field name="domain_force">[('permission_read', '=', user.id)]</field>
</record>
<record id="rule_file_computed_unlink" model="ir.rule">
<field name="name">Apply computed unlink permissions.</field>
<field name="model_id" ref="model_dms_file" />
<field name="global" eval="True" />
<field name="perm_read" eval="0" />
<field name="perm_create" eval="0" />
<field name="perm_write" eval="0" />
<field name="perm_unlink" eval="1" />
<field name="domain_force">[('permission_unlink', '=', user.id)]</field>
</record>
<record id="rule_file_computed_write" model="ir.rule">
<field name="name">Apply computed write permissions.</field>
<field name="model_id" ref="model_dms_file" />
<field name="global" eval="True" />
<field name="perm_read" eval="0" />
<field name="perm_create" eval="0" />
<field name="perm_write" eval="1" />
<field name="perm_unlink" eval="0" />
<field name="domain_force">[('permission_write', '=', user.id)]</field>
</record>
</odoo>
17 changes: 16 additions & 1 deletion dms/tests/test_directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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()
Expand Down
45 changes: 45 additions & 0 deletions dms/tests/test_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
Loading
Loading