From 7a529dcaf954281962b2b77c40cac7d883eff974 Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Wed, 13 May 2026 23:50:34 +0200 Subject: [PATCH 1/3] [IMP] base_tier_validation: warn at definition save if reviewer lacks model ACL Make misconfigured tier definitions visible at config time instead of silently producing reviews nobody can act on. When the admin sets a reviewer (individual user or group) that has no ir.model.access read on the tier's target model, an onchange warning explains the problem and names the affected user(s) -- but does not block the save. Legitimate workflows (the user is about to be added to the group, ir.rules grant per-record access, ...) still go through. Skipped for review_type='field': the reviewer only resolves at validation time from a field on the document, so we can't check it in advance. The chatter notification at request_validation time (separate PR) catches that case. The helper deliberately checks only model-level ACL: per-record ir.rule restrictions can grant or revoke access at runtime, so the warning is worded as "may not be able to read" rather than asserting a definite block. False positives are preferable to silent failure. Tests cover all three branches (individual, group, field). --- .../models/tier_definition.py | 66 +++++++++++++++++++ .../tests/test_tier_validation.py | 56 ++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/base_tier_validation/models/tier_definition.py b/base_tier_validation/models/tier_definition.py index b36a82f69..71f74b332 100644 --- a/base_tier_validation/models/tier_definition.py +++ b/base_tier_validation/models/tier_definition.py @@ -2,6 +2,7 @@ # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). from odoo import api, fields, models +from odoo.exceptions import AccessError from odoo.fields import Domain @@ -115,6 +116,71 @@ def onchange_review_type(self): self.reviewer_id = None self.reviewer_group_id = None + def _reviewers_without_model_access(self, model_name, users): + """Return the subset of ``users`` that cannot read ``model_name``. + + The check is model-level only (``ir.model.access``). It does not + evaluate per-record ``ir.rule`` restrictions, so callers should + treat a non-empty result as "*probably* no access" rather than a + guarantee -- record rules can grant or revoke access dynamically + at runtime. + """ + Model = self.env.get(model_name) + if Model is None: + return self.env["res.users"] + no_access = self.env["res.users"] + for user in users: + try: + Model.with_user(user).check_access("read") + except AccessError: + no_access |= user + return no_access + + def _reviewers_to_check_for_access(self): + """Return the user recordset whose model access we can check ahead + of time for this definition. Skip review types where the reviewer + only resolves at validation time (``field`` reads off the record).""" + self.ensure_one() + if self.review_type == "individual": + return self.reviewer_id + if self.review_type == "group": + return self.reviewer_group_id.user_ids + return self.env["res.users"] + + @api.onchange("review_type", "reviewer_id", "reviewer_group_id", "model_id") + def _onchange_warn_reviewer_access(self): + """Advisory warning when an assigned reviewer cannot read the model. + + Non-blocking: legitimate workflows (admin is about to grant the + group, ir.rules expose specific records, ...) still save. The + warning just makes it impossible to misconfigure this silently. + """ + if not (self.model and self.review_type): + return + users = self._reviewers_to_check_for_access() + if not users: + return + no_access = self._reviewers_without_model_access(self.model, users) + if not no_access: + return + return { + "warning": { + "title": self.env._("Reviewer may lack access"), + "message": self.env._( + "The following reviewer(s) may not be able to read " + "'%(model)s' records and so cannot act on the reviews " + "this tier will create: %(reviewers)s.\n\n" + "This is a best-effort check against model-level access " + "rights only -- record rules may still grant or revoke " + "access at runtime. Make sure these users belong to a " + "group with read access on the target model, or pick " + "different reviewers.", + model=self.model_id.name or self.model, + reviewers=", ".join(no_access.mapped("display_name")), + ), + } + } + @api.depends("review_type", "model_id") def _compute_domain_reviewer_field(self): models = self.mapped("model") diff --git a/base_tier_validation/tests/test_tier_validation.py b/base_tier_validation/tests/test_tier_validation.py index 3f2ab80cf..0f349e045 100644 --- a/base_tier_validation/tests/test_tier_validation.py +++ b/base_tier_validation/tests/test_tier_validation.py @@ -536,6 +536,62 @@ def test_16b_review_user_count_no_model_access(self): result = self.test_user_2.with_user(self.test_user_2).review_user_count() self.assertEqual(result, []) + def test_definition_onchange_warns_when_reviewer_lacks_access(self): + """Setting an individual reviewer with no read access on the target + model returns a non-blocking onchange warning.""" + # Sanity: with the default public ACL, no warning. + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "individual", + "reviewer_id": self.test_user_2.id, + } + ) + self.assertIsNone(definition._onchange_warn_reviewer_access()) + # Revoke read access on the validated model for non-superadmin users. + self.env["ir.model.access"].search( + Domain("model_id", "=", self.tester_model.id) + ).unlink() + warning = definition._onchange_warn_reviewer_access() + self.assertIsNotNone(warning) + self.assertIn(self.test_user_2.display_name, warning["warning"]["message"]) + + def test_definition_onchange_warns_when_group_member_lacks_access(self): + """Group reviewer: if any member of the assigned group lacks read + access on the target model, the onchange warns and names them.""" + # Put test_user_2 in a fresh group; revoke the public ACL. + group = self.env["res.groups"].create( + {"name": "Tier Reviewers", "user_ids": [Command.link(self.test_user_2.id)]} + ) + self.env["ir.model.access"].search( + Domain("model_id", "=", self.tester_model.id) + ).unlink() + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "group", + "reviewer_group_id": group.id, + } + ) + warning = definition._onchange_warn_reviewer_access() + self.assertIsNotNone(warning) + self.assertIn(self.test_user_2.display_name, warning["warning"]["message"]) + + def test_definition_onchange_skips_field_review_type(self): + """The 'field' review type cannot be checked ahead of time -- the + reviewer only resolves at validation time -- so the onchange + returns no warning even if the ACL would block.""" + self.env["ir.model.access"].search( + Domain("model_id", "=", self.tester_model.id) + ).unlink() + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "field", + } + ) + self.assertIsNone(definition._onchange_warn_reviewer_access()) + def test_17_search_records_no_validation(self): """Search for records that have no validation process started""" records = self.env["tier.validation.tester"].search( From 4ceb68fc143fda5382b8316df20633e7ec8f2f21 Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Thu, 14 May 2026 09:27:45 +0200 Subject: [PATCH 2/3] [FIX] base_tier_validation: tests for definition-time access warning Two issues in the onchange tests: - Test setup unlinked the public tier.validation.tester ACL after the .new() definition had already been built and the onchange had already called check_access once. The ir.model.access ormcache retained the earlier "allowed" decision, so the second onchange call still saw the reviewer as having access. Revoke the ACL before constructing the .new() record, and call ir.model.access.call_cache_clearing_methods() to be sure subsequent check_access invocations re-evaluate. Extract to a helper used by all three onchange tests so they exercise the same path. - On .new() records, the stored related ``self.model`` field can still be empty depending on cache state, which made the onchange silently return None in the failing CI environment. Read the model name directly off ``self.model_id.model`` (the m2o navigation) so it's always populated when ``model_id`` has been set. --- .../models/tier_definition.py | 10 ++++--- .../tests/test_tier_validation.py | 26 ++++++++++--------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/base_tier_validation/models/tier_definition.py b/base_tier_validation/models/tier_definition.py index 71f74b332..cdcd2dc14 100644 --- a/base_tier_validation/models/tier_definition.py +++ b/base_tier_validation/models/tier_definition.py @@ -155,12 +155,16 @@ def _onchange_warn_reviewer_access(self): group, ir.rules expose specific records, ...) still save. The warning just makes it impossible to misconfigure this silently. """ - if not (self.model and self.review_type): + # Read the model name off the m2o directly rather than via the + # stored related ``self.model`` -- on ``.new()`` records the + # related can still be empty depending on cache state. + model_name = self.model_id.model + if not (model_name and self.review_type): return users = self._reviewers_to_check_for_access() if not users: return - no_access = self._reviewers_without_model_access(self.model, users) + no_access = self._reviewers_without_model_access(model_name, users) if not no_access: return return { @@ -175,7 +179,7 @@ def _onchange_warn_reviewer_access(self): "access at runtime. Make sure these users belong to a " "group with read access on the target model, or pick " "different reviewers.", - model=self.model_id.name or self.model, + model=self.model_id.name or model_name, reviewers=", ".join(no_access.mapped("display_name")), ), } diff --git a/base_tier_validation/tests/test_tier_validation.py b/base_tier_validation/tests/test_tier_validation.py index 0f349e045..60dbe83fb 100644 --- a/base_tier_validation/tests/test_tier_validation.py +++ b/base_tier_validation/tests/test_tier_validation.py @@ -536,10 +536,21 @@ def test_16b_review_user_count_no_model_access(self): result = self.test_user_2.with_user(self.test_user_2).review_user_count() self.assertEqual(result, []) + def _revoke_tester_model_access(self): + """Make the tester model unreadable for non-admin users by + unlinking its public ACL and clearing the ACL cache so the next + check_access actually re-evaluates against the new state.""" + self.env["ir.model.access"].search( + Domain("model_id", "=", self.tester_model.id) + ).unlink() + self.env["ir.model.access"].call_cache_clearing_methods() + def test_definition_onchange_warns_when_reviewer_lacks_access(self): """Setting an individual reviewer with no read access on the target model returns a non-blocking onchange warning.""" - # Sanity: with the default public ACL, no warning. + # Revoke first so the per-record check_access cache for test_user_2 + # never gets populated with a stale "allowed" result. + self._revoke_tester_model_access() definition = self.tier_def_obj.new( { "model_id": self.tester_model.id, @@ -547,11 +558,6 @@ def test_definition_onchange_warns_when_reviewer_lacks_access(self): "reviewer_id": self.test_user_2.id, } ) - self.assertIsNone(definition._onchange_warn_reviewer_access()) - # Revoke read access on the validated model for non-superadmin users. - self.env["ir.model.access"].search( - Domain("model_id", "=", self.tester_model.id) - ).unlink() warning = definition._onchange_warn_reviewer_access() self.assertIsNotNone(warning) self.assertIn(self.test_user_2.display_name, warning["warning"]["message"]) @@ -563,9 +569,7 @@ def test_definition_onchange_warns_when_group_member_lacks_access(self): group = self.env["res.groups"].create( {"name": "Tier Reviewers", "user_ids": [Command.link(self.test_user_2.id)]} ) - self.env["ir.model.access"].search( - Domain("model_id", "=", self.tester_model.id) - ).unlink() + self._revoke_tester_model_access() definition = self.tier_def_obj.new( { "model_id": self.tester_model.id, @@ -581,9 +585,7 @@ def test_definition_onchange_skips_field_review_type(self): """The 'field' review type cannot be checked ahead of time -- the reviewer only resolves at validation time -- so the onchange returns no warning even if the ACL would block.""" - self.env["ir.model.access"].search( - Domain("model_id", "=", self.tester_model.id) - ).unlink() + self._revoke_tester_model_access() definition = self.tier_def_obj.new( { "model_id": self.tester_model.id, From 83e12ee680fcb94536b6ead262140d57d183cf11 Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Thu, 14 May 2026 17:37:09 +0200 Subject: [PATCH 3/3] [IMP] base_tier_validation: cover onchange happy-path early returns --- .../tests/test_tier_validation.py | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/base_tier_validation/tests/test_tier_validation.py b/base_tier_validation/tests/test_tier_validation.py index 60dbe83fb..00c3958b2 100644 --- a/base_tier_validation/tests/test_tier_validation.py +++ b/base_tier_validation/tests/test_tier_validation.py @@ -594,6 +594,47 @@ def test_definition_onchange_skips_field_review_type(self): ) self.assertIsNone(definition._onchange_warn_reviewer_access()) + def test_definition_onchange_returns_nothing_when_no_problem(self): + """The onchange must stay silent when there's nothing to warn + about. Covers the three early-return branches: + + - no model selected yet (``model_id`` empty); + - no reviewer set yet on a model that has one; + - reviewer has model access (default ACL in place). + """ + # No model -> early return on `if not (model_name and ...):`. + definition = self.tier_def_obj.new({"review_type": "individual"}) + self.assertIsNone(definition._onchange_warn_reviewer_access()) + # No reviewer -> early return on `if not users:`. + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "individual", + } + ) + self.assertIsNone(definition._onchange_warn_reviewer_access()) + # Reviewer with access (default public ACL is in place) -> the + # access check finds nothing to flag and the onchange returns + # None at the `if not no_access:` exit. + definition = self.tier_def_obj.new( + { + "model_id": self.tester_model.id, + "review_type": "individual", + "reviewer_id": self.test_user_2.id, + } + ) + self.assertIsNone(definition._onchange_warn_reviewer_access()) + + def test_reviewers_without_model_access_unknown_model(self): + """When the target model name doesn't resolve (e.g. a stale + reference after an uninstall), the helper returns an empty + res.users recordset without raising.""" + result = self.tier_def_obj._reviewers_without_model_access( + "this.model.does.not.exist", self.test_user_2 + ) + self.assertFalse(result) + self.assertEqual(result._name, "res.users") + def test_17_search_records_no_validation(self): """Search for records that have no validation process started""" records = self.env["tier.validation.tester"].search(