fix(spp_alerts): evaluate alert rules as their owner, not the elevated cron - #364
fix(spp_alerts): evaluate alert rules as their owner, not the elevated cron#364gonzalesedwin1123 wants to merge 6 commits into
Conversation
The alert-rule evaluation cron runs with an elevated identity (ir.cron's default user is the installer/superuser). The monitored search ran with that identity, so a rule authored by a non-system-admin group_alerts_manager could surface records the author is not allowed to see; the resulting alerts, readable by all alert managers, leaked data across the record-rule boundary. Evaluate each rule's monitored search as the rule's owner (create_uid) via with_user(), so record rules are enforced against whoever configured the rule regardless of who (or what cron) triggers evaluation. An admin-authored rule keeps its wider scope. Adds tests proving the elevated cron no longer surfaces owner-hidden records, and that an unrestricted owner's rule still spans all records.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #364 +/- ##
==========================================
- Coverage 74.28% 71.06% -3.22%
==========================================
Files 372 134 -238
Lines 25385 11516 -13869
==========================================
- Hits 18857 8184 -10673
+ Misses 6528 3332 -3196
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
create_uid is an ORM-readonly system field (not user input), so switching the monitored search to it is safe despite odoo-with-user-unvalidated. Also regenerate README.rst/index.html from the HISTORY fragment (CI oca-gen).
Keying evaluation solely on create_uid closed the create vector but not the edit vector: group_alerts_manager has model-wide write on spp.alert.rule and no record rule confines managers to their own rules, so a non-admin manager could repoint an admin-authored rule (change model/domain/monitored field) and have it still evaluate as the admin create_uid — re-opening the leak. Add a system-managed eval_as_user_id (default create_uid) that is re-bound to the editor whenever a targeting field (model_id, domain_filter, monitored_field_id, date_field_id, rule_type) changes, and never client-writable (stripped from create/write vals). Evaluate the monitored search as eval_as_user_id so record rules always bound a rule to whoever last defined its targeting. Ship a migration backfilling eval_as_user_id = create_uid for existing rules. Adds tests: manager repointing an unrestricted-owned rule de-escalates to the manager; editing a non-targeting field preserves owner scope; eval_as_user_id cannot be forged via create or write.
Addresses two-reviewer adversarial staff review of the A2 fix: - HIGH (forge via context): create() force-sets eval_as_user_id instead of popping it, so the field is always present and Odoo default_get never honours a client default_eval_as_user_id context key (also covers copy()). - HIGH (migration no-op): drop the Python default (which made _init_column pre-fill existing rows with the upgrade user before the migration) and make the backfill unconditional (SET eval_as_user_id = create_uid) so it is authoritative. - MED: expand _EVAL_TARGETING_FIELDS with comparison, threshold_value, days_before, active — fields that change what a rule surfaces without touching model/domain. - MED: fail closed — if no eval user resolves, skip the rule instead of searching with the elevated cron identity. - MED: evaluate in the owner's own company scope, not the triggering cron's default company, so multi-company record rules apply as they would for that user. - Low: write() copies vals (no caller mutation); comment that eval identity is the acting user (preserved under sudo). Adds tests: context-default forge (create + copy), threshold/active rebind, fail-closed, owner-company scoping, and the shipped migration's authoritative backfill. 118 tests, 0 failed.
Staff review (two independent adversarial reviewers) — NEEDS-CHANGES, all fixedA security-completeness reviewer (trying to break the fix) and a regression/correctness reviewer (Odoo 19 + upgrade behavior) reviewed the diff independently. Both returned NEEDS-CHANGES; each found one HIGH bug the other missed. All findings fixed in HIGH — evaluation identity forgeable via context default. HIGH — migration was a no-op on real upgrades. The field carried a Python MEDIUM:
Low: New tests cover every finding: context-default forge (create + copy), threshold/re-activation rebind, fail-closed, owner-company scoping, and the shipped Both reviewers confirmed clean: the read path (no re-elevation between search and alert values), the |
|
Follow-up filed: #374 — |
2 participants
Summary
Alert rules are evaluated by a scheduled cron running with an elevated identity (
ir.cron's default user is the installer/superuser). The monitored search (spp.alert.rule._evaluate_rule→Model.search(domain)) ran with that identity, so record rules were bypassed for whatever a rule targeted.group_alerts_manager— a delegated, non-base.group_systemrole — has full CRUD onspp.alert.rule, rules can target any model with any domain, and no record rule confines managers to their own rules. So a non-admin Alerts Manager could author, or repoint an admin-authored rule, over e.g.res.partner; the elevated cron evaluated it past the record rules that scope their visibility; and the resulting alerts (readable by all alert managers) leaked record values + names across the boundary, one rule at a time.Fix
Evaluate each rule's monitored search as the user who configured what the rule targets, tracked by a new system-managed
eval_as_user_id:create/writevalues (not forgeable).write()whenever a targeting field changes (model_id,domain_filter,monitored_field_id,date_field_id,rule_type) — so repointing an admin-authored rule de-escalates it to the editor. Editing a non-targeting field (name, priority, …) leaves ownership untouched._evaluate_ruleruns the search viawith_user((eval_as_user_id or create_uid).id), so record rules always bound a rule to whoever last defined its targeting, regardless of the elevated cron. An admin-authored rule keeps its wider scope.Keying evaluation on
create_uidalone would have closed the create vector but not the repoint an existing higher-privileged rule vector (managers have model-wide write) — hence the re-bind-on-targeting-change design.Migration:
migrations/19.0.2.0.1/post-migration.pybackfillseval_as_user_id = create_uidfor existing rules (module is released).Tests (
spp_alerts/tests/test_rule_evaluation_access.py, TDD)test_elevated_eval_does_not_surface_owner_hidden_records/test_cron_path_respects_owner_record_rules— repro: elevated eval no longer surfaces records the owner can't see, via the direct and cron entry points.test_manager_repointing_owner_rule_de_escalates— a manager repointing an unrestricted-owned rule re-binds to the manager; the hidden record is not leaked.test_manager_editing_nontargeting_field_preserves_owner_scope— editing a non-targeting field preserves the owner's scope.test_eval_as_user_id_not_client_writable— cannot be forged via create or write.test_manager_cannot_see_hidden_partner(sanity),test_unrestricted_owner_rule_still_spans_all_records(preserved system-wide scope).Full module suite green: 111 tests, 0 failed.