Skip to content

Commit 7973bef

Browse files
Merge pull request #290 from OpenSPP/fix/spp-audit-disable-context
perf(spp_audit): audit_disable context bypass + skip snapshot reads when no rule matches
2 parents 47b12f1 + e182862 commit 7973bef

7 files changed

Lines changed: 225 additions & 21 deletions

File tree

spp_audit/README.rst

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,19 @@ External Python: ``requests`` (HTTP backend)
145145
Changelog
146146
=========
147147

148+
19.0.2.0.2
149+
~~~~~~~~~~
150+
151+
- feat: ``audit_disable`` context key lets trusted machine flows (e.g.
152+
cross-instance replication of records already audited at their source)
153+
bypass audit logging and its full-record snapshot reads in
154+
create/write/unlink
155+
- perf: skip the full-record snapshot ``read(load="_classic_write")``
156+
when no audit rule matches the method — ``audit_create`` read
157+
unconditionally and ``audit_write`` did its post-write read even with
158+
zero matching rules, evaluating every non-stored compute (with
159+
per-record searches) on every create/write of an audited model
160+
148161
19.0.2.0.1
149162
~~~~~~~~~~
150163

spp_audit/__manifest__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"name": "OpenSPP Audit",
44
"summary": "Comprehensively tracks all data modifications and user actions across the OpenSPP platform, recording old and new values for configured data. It enhances accountability and data integrity by maintaining an immutable history of changes, crucial for internal audits, compliance, and detecting unauthorized alterations. Supports multiple backends (database, file, syslog, HTTP) with tamper-resistant configuration.",
55
"category": "OpenSPP/Monitoring",
6-
"version": "19.0.2.0.1",
6+
"version": "19.0.2.0.2",
77
"sequence": 1,
88
"author": "OpenSPP.org",
99
"website": "https://github.com/OpenSPP/OpenSPP2",

spp_audit/readme/HISTORY.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
### 19.0.2.0.2
2+
3+
- feat: `audit_disable` context key lets trusted machine flows (e.g. cross-instance replication of records already audited at their source) bypass audit logging and its full-record snapshot reads in create/write/unlink
4+
- perf: skip the full-record snapshot `read(load="_classic_write")` when no audit rule matches the method — `audit_create` read unconditionally and `audit_write` did its post-write read even with zero matching rules, evaluating every non-stored compute (with per-record searches) on every create/write of an audited model
5+
16
### 19.0.2.0.1
27

38
- fix: use @api.model_create_multi for audit_create to support Odoo 19 create overrides (#138)

spp_audit/static/description/index.html

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,20 @@ <h2><a class="toc-backref" href="#toc-entry-1">Changelog</a></h2>
516516
</div>
517517
</div>
518518
<div class="section" id="section-1">
519+
<h1>19.0.2.0.2</h1>
520+
<ul class="simple">
521+
<li>feat: <tt class="docutils literal">audit_disable</tt> context key lets trusted machine flows (e.g.
522+
cross-instance replication of records already audited at their source)
523+
bypass audit logging and its full-record snapshot reads in
524+
create/write/unlink</li>
525+
<li>perf: skip the full-record snapshot <tt class="docutils literal"><span class="pre">read(load=&quot;_classic_write&quot;)</span></tt>
526+
when no audit rule matches the method — <tt class="docutils literal">audit_create</tt> read
527+
unconditionally and <tt class="docutils literal">audit_write</tt> did its post-write read even with
528+
zero matching rules, evaluating every non-stored compute (with
529+
per-record searches) on every create/write of an audited model</li>
530+
</ul>
531+
</div>
532+
<div class="section" id="section-2">
519533
<h1>19.0.2.0.1</h1>
520534
<ul class="simple">
521535
<li>fix: use &#64;api.model_create_multi for audit_create to support Odoo 19
@@ -528,7 +542,7 @@ <h1>19.0.2.0.1</h1>
528542
<tt class="docutils literal">parent_data_html</tt> fields to prevent stored XSS (#50)</li>
529543
</ul>
530544
</div>
531-
<div class="section" id="section-2">
545+
<div class="section" id="section-3">
532546
<h1>19.0.2.0.0</h1>
533547
<ul class="simple">
534548
<li>Initial migration to OpenSPP2</li>

spp_audit/tests/__init__.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
from . import test_audit_backend
2+
from . import test_audit_disable
23
from . import test_html_escaping
34
from . import test_spp_audit_rule
Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
from unittest.mock import patch
2+
3+
from odoo import Command
4+
from odoo.tests.common import TransactionCase
5+
6+
7+
class AuditDisableCommon(TransactionCase):
8+
"""Shared fixture: a res.partner audit rule with all methods logged."""
9+
10+
@classmethod
11+
def setUpClass(cls):
12+
super().setUpClass()
13+
cls.partner_model = cls.env["ir.model"].search([("model", "=", "res.partner")], limit=1)
14+
cls.audit_rule = cls.env["spp.audit.rule"].search([("model_id", "=", cls.partner_model.id)], limit=1)
15+
if not cls.audit_rule:
16+
cls.audit_rule = cls.env["spp.audit.rule"].create(
17+
{
18+
"name": "Audit Disable Test Rule",
19+
"model_id": cls.partner_model.id,
20+
"is_log_create": True,
21+
"is_log_write": True,
22+
"is_log_unlink": True,
23+
}
24+
)
25+
else:
26+
cls.audit_rule.write({"is_log_create": True, "is_log_write": True, "is_log_unlink": True})
27+
28+
def _logs(self, records, method):
29+
return self.env["spp.audit.log"].search(
30+
[
31+
("model_id", "=", self.partner_model.id),
32+
("method", "=", method),
33+
("res_id", "in", records.ids),
34+
]
35+
)
36+
37+
def _classic_write_read_spy(self):
38+
"""Context manager patching res.partner.read to record full-record
39+
(load="_classic_write") reads — the decorator's snapshot reads."""
40+
partner_cls = type(self.env["res.partner"])
41+
real_read = partner_cls.read
42+
reads = []
43+
44+
def spy(records, fields=None, load="lazy"):
45+
if load == "_classic_write":
46+
reads.append(records.ids)
47+
return real_read(records, fields, load=load)
48+
49+
return patch.object(partner_cls, "read", spy), reads
50+
51+
52+
class TestAuditDisableContext(AuditDisableCommon):
53+
"""audit_disable context: machine flows (e.g. cross-instance replication
54+
of records already audited at their source) must be able to bypass audit
55+
logging AND its full-record snapshot reads entirely."""
56+
57+
def test_create_bypassed(self):
58+
patcher, reads = self._classic_write_read_spy()
59+
with patcher:
60+
partner = self.env["res.partner"].with_context(audit_disable=True).create({"name": "Disable Create"})
61+
self.assertTrue(partner.exists())
62+
self.assertFalse(self._logs(partner, "create"))
63+
self.assertFalse(reads, "audit_disable must skip the full-record snapshot read on create")
64+
65+
def test_write_bypassed(self):
66+
partner = self.env["res.partner"].create({"name": "Disable Write"})
67+
patcher, reads = self._classic_write_read_spy()
68+
with patcher:
69+
partner.with_context(audit_disable=True).write({"name": "Disable Write Changed"})
70+
self.assertEqual(partner.name, "Disable Write Changed")
71+
self.assertFalse(self._logs(partner, "write"))
72+
self.assertFalse(reads, "audit_disable must skip the full-record snapshot reads on write")
73+
74+
def test_unlink_bypassed(self):
75+
partner = self.env["res.partner"].create({"name": "Disable Unlink"})
76+
partner_id = partner.id
77+
patcher, reads = self._classic_write_read_spy()
78+
with patcher:
79+
partner.with_context(audit_disable=True).unlink()
80+
self.assertFalse(partner.exists())
81+
self.assertFalse(
82+
self._logs(self.env["res.partner"].browse(partner_id), "unlink"),
83+
"audit_disable must skip the unlink log",
84+
)
85+
self.assertFalse(reads, "audit_disable must skip the full-record snapshot read on unlink")
86+
87+
def test_without_flag_still_audited(self):
88+
# Control: the same operations without the flag keep their audit trail.
89+
partner = self.env["res.partner"].create({"name": "Still Audited"})
90+
partner.write({"name": "Still Audited Changed"})
91+
self.assertTrue(self._logs(partner, "create"))
92+
self.assertTrue(self._logs(partner, "write"))
93+
94+
95+
class TestNoMatchingRuleNoRead(AuditDisableCommon):
96+
"""With no rule matching the method, the decorator must not pay the
97+
full-record snapshot read — previously audit_create read unconditionally
98+
and audit_write did its post-write read even with zero matching rules."""
99+
100+
def test_create_without_create_rule(self):
101+
self.audit_rule.write({"is_log_create": False})
102+
patcher, reads = self._classic_write_read_spy()
103+
with patcher:
104+
partner = self.env["res.partner"].create({"name": "No Create Rule"})
105+
self.assertTrue(partner.exists())
106+
self.assertFalse(self._logs(partner, "create"))
107+
self.assertFalse(reads, "no create rule -> no full-record snapshot read")
108+
109+
def test_write_without_write_rule(self):
110+
partner = self.env["res.partner"].create({"name": "No Write Rule"})
111+
self.audit_rule.write({"is_log_write": False})
112+
patcher, reads = self._classic_write_read_spy()
113+
with patcher:
114+
partner.write({"name": "No Write Rule Changed"})
115+
self.assertEqual(partner.name, "No Write Rule Changed")
116+
self.assertFalse(self._logs(partner, "write"))
117+
self.assertFalse(reads, "no write rule -> no full-record snapshot reads")
118+
119+
def test_unlink_without_unlink_rule(self):
120+
partner = self.env["res.partner"].create({"name": "No Unlink Rule"})
121+
partner_id = partner.id
122+
self.audit_rule.write({"is_log_unlink": False})
123+
patcher, reads = self._classic_write_read_spy()
124+
with patcher:
125+
partner.unlink()
126+
self.assertFalse(partner.exists())
127+
self.assertFalse(
128+
self._logs(self.env["res.partner"].browse(partner_id), "unlink"),
129+
"no unlink rule -> no unlink log",
130+
)
131+
self.assertFalse(reads, "no unlink rule -> no full-record snapshot read")
132+
133+
134+
class TestMarkupValuesLogged(AuditDisableCommon):
135+
"""Markup field values (Html fields, e.g. res.partner.comment) must be
136+
stringified before the audit payload is logged."""
137+
138+
def test_html_field_value_is_stringified_in_log(self):
139+
# Make sure the Html field is among the rule's logged fields (a
140+
# pre-existing rule may restrict field_to_log_ids to a fixed list).
141+
comment_field = self.env["ir.model.fields"]._get("res.partner", "comment")
142+
self.audit_rule.write({"field_to_log_ids": [Command.link(comment_field.id)]})
143+
partner = self.env["res.partner"].create(
144+
{
145+
"name": "Markup Partner",
146+
"comment": "<p>Audit Markup Body</p>",
147+
}
148+
)
149+
logs = self._logs(partner, "create")
150+
self.assertTrue(logs, "create with an Html field must still be logged")
151+
self.assertIn(
152+
"Audit Markup Body",
153+
logs[0].data or "",
154+
"the Html field's content must appear in the logged data as plain text",
155+
)

spp_audit/tools/decorator.py

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,14 @@
1111
_logger = logging.getLogger(__name__)
1212

1313

14+
def _stringify_markup(values):
15+
"""Convert Markup field values to plain str in read() result dicts."""
16+
for rec_values in values or []:
17+
for key, value in rec_values.items():
18+
if isinstance(value, Markup):
19+
rec_values[key] = str(value)
20+
21+
1422
def audit_decorator(method):
1523
"""
1624
The audit_decorator function is a Python decorator that adds auditing functionality to create, write, and
@@ -24,9 +32,18 @@ def audit_decorator(method):
2432

2533
@api.model_create_multi
2634
def audit_create(self, vals_list):
35+
# audit_disable: trusted machine flows (e.g. replication of records
36+
# already audited at their source) opt out of audit logging and its
37+
# full-record snapshot reads.
38+
if self.env.context.get("audit_disable"):
39+
return audit_create.origin(self, vals_list)
2740
result = audit_create.origin(self, vals_list)
2841
records = result
2942
rules = self.get_audit_rules("create")
43+
if not rules:
44+
# No matching rule: skip the snapshot read — reading every field
45+
# evaluates every non-stored compute on the new records.
46+
return result
3047

3148
# Use sudo() to avoid access errors when reading computed fields
3249
new_values = (
@@ -37,15 +54,14 @@ def audit_create(self, vals_list):
3754
)
3855
)
3956
if new_values:
40-
for nv in new_values:
41-
for key, value in nv.items():
42-
if isinstance(value, Markup):
43-
nv[key] = str(value)
44-
57+
_stringify_markup(new_values)
4558
rules.log("create", new_values=new_values)
4659
return result
4760

4861
def audit_write(self, vals):
62+
# audit_disable: trusted machine flows opt out entirely (see audit_create)
63+
if self.env.context.get("audit_disable"):
64+
return audit_write.origin(self, vals)
4965
# Prevent recursive audit logging from computed field updates
5066
if self.env.context.get("audit_in_progress"):
5167
return audit_write.origin(self, vals)
@@ -65,27 +81,31 @@ def audit_write(self, vals):
6581
# Set flag to prevent recursive auditing
6682
result = audit_write.origin(self.with_context(audit_in_progress=True), vals)
6783

84+
if not rules:
85+
# No matching rule: skip the post-write snapshot read too — it
86+
# previously ran unconditionally, evaluating every non-stored
87+
# compute on every write of an audited model.
88+
return result
89+
6890
new_values = (
6991
self.sudo() # nosemgrep: odoo-sudo-without-context
7092
.with_context(allowed_company_ids=[])
7193
.read(load="_classic_write")
7294
)
7395

7496
if new_values and old_values_copy:
75-
for nv in new_values:
76-
for key, value in nv.items():
77-
if isinstance(value, Markup):
78-
nv[key] = str(value)
79-
for ov in old_values_copy:
80-
for key, value in ov.items():
81-
if isinstance(value, Markup):
82-
ov[key] = str(value)
83-
97+
_stringify_markup(new_values)
98+
_stringify_markup(old_values_copy)
8499
rules.log("write", old_values_copy, new_values)
85100
return result
86101

87102
def audit_unlink(self):
103+
# audit_disable: trusted machine flows opt out entirely (see audit_create)
104+
if self.env.context.get("audit_disable"):
105+
return audit_unlink.origin(self)
88106
rules = self.get_audit_rules("unlink")
107+
if not rules:
108+
return audit_unlink.origin(self)
89109
# Use sudo() to avoid access errors when reading computed fields
90110
old_values = (
91111
self.sudo() # nosemgrep: odoo-sudo-without-context
@@ -94,11 +114,7 @@ def audit_unlink(self):
94114
)
95115

96116
if old_values:
97-
for ov in old_values:
98-
for key, value in ov.items():
99-
if isinstance(value, Markup):
100-
ov[key] = str(value)
101-
117+
_stringify_markup(old_values)
102118
rules.log("unlink", old_values)
103119
return audit_unlink.origin(self)
104120

0 commit comments

Comments
 (0)