feat(spp_alerts): add rule evaluation engine and promote to Beta#12
feat(spp_alerts): add rule evaluation engine and promote to Beta#12
Conversation
Add generic rule evaluation engine to spp_alerts so alert rules can automatically create alerts based on threshold and date conditions. Rules can be evaluated via daily cron or manually with a Run Now button. Key changes: - Add rule_type, domain_filter, monitored_field_id, date_field_id, comparison fields to spp.alert.rule - Add rule_id, res_model, res_id source tracking fields to spp.alert - Implement _evaluate_rule, _evaluate_threshold, _evaluate_date methods with batch duplicate prevention - Add daily cron job for automatic rule evaluation - Add Run Now button on rule form for manual evaluation - UI improvements: readonly fields when resolved, required resolution notes, searchpanel fix, title styling, created date display - Resolve button only visible after acknowledging - Promote development_status from Alpha to Beta - Add 20 test cases for rule evaluation engine
Summary of ChangesHello @emjay0921, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully adds a much-needed rule evaluation engine to the spp_alerts module, promoting it to Beta status. However, a significant security vulnerability was identified: the engine uses sudo() when searching for records matching a user-defined domain, which allows users with 'Alert Manager' privileges to bypass system-wide access controls and potentially exfiltrate sensitive information. This critical issue must be addressed by ensuring record searches respect appropriate access rights. Additionally, there are a couple of minor suggestions to further improve code clarity and test conciseness.
spp_alerts/models/alert_rule.py
Outdated
| _logger.error("Alert rule '%s': invalid domain filter '%s': %s", self.name, self.domain_filter, e) | ||
| return 0 | ||
|
|
||
| records = Model.sudo().search(domain) |
There was a problem hiding this comment.
The use of sudo().search(domain) here is a security risk because domain is derived from the user-configurable domain_filter field. A user with 'Alert Manager' privileges (who can create and edit alert rules) can craft a malicious domain to query any model in the system, bypassing all Access Control Lists (ACLs) and Record Rules. This allows for unauthorized information disclosure of sensitive data from any model by observing the creation of alerts.
To remediate this, remove the .sudo() call so that the search respects the current user's access rights. If this is intended to run as a background cron, the search will naturally use the permissions of the user configured for the cron job.
| records = Model.sudo().search(domain) | |
| records = Model.search(domain) |
spp_alerts/models/alert_rule.py
Outdated
| if hasattr(date_value, "date"): | ||
| date_value = date_value.date() |
There was a problem hiding this comment.
Using hasattr(date_value, "date") for type checking is a bit fragile as it relies on duck typing. A more robust and explicit approach is to use isinstance to check if the value is a datetime object. This makes the code's intent clearer and prevents potential errors.
To apply the suggestion, you'll also need to add from datetime import datetime to the imports at the top of the file.
| if hasattr(date_value, "date"): | |
| date_value = date_value.date() | |
| if isinstance(date_value, datetime): | |
| date_value = date_value.date() |
| alert.write({"resolution_notes": "Test resolution"}) | ||
| alert.action_resolve(notes="Test resolution") |
There was a problem hiding this comment.
This test contains a redundant step. The call to alert.write() on line 255 is unnecessary because action_resolve(notes=...) on the next line already handles setting the resolution notes. You can simplify these two lines into a single call to action_resolve to make the test cleaner and more focused.
| alert.write({"resolution_notes": "Test resolution"}) | |
| alert.action_resolve(notes="Test resolution") | |
| alert.action_resolve(notes="Test resolution") |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…y fields - Update test_action_resolve_without_notes to expect UserError (resolution notes are now required) - Use color (integer) instead of credit_limit (requires account module) for threshold tests to avoid dependency on optional modules - Use write_date instead of date for date rule tests
- Remove all sudo() calls from rule evaluation engine (security) - Use isinstance(datetime) instead of hasattr for type checking - Add nosemgrep for standard ir.sequence.sudo() pattern - Remove unused variable in test_alert_mail_tracking - Add extension points to form views for extensibility - Auto-format XML via prettier
| # Use sudo to access sequence (users may not have ir.sequence access) | ||
| vals["reference"] = self.env["ir.sequence"].sudo().next_by_code("spp.alert") or _("New") | ||
| # nosemgrep: odoo-sudo-without-context - Standard sequence access | ||
| Sequence = self.env["ir.sequence"].sudo() |
Check warning
Code scanning / Semgrep OSS
Semgrep Finding: semgrep.odoo-sudo-without-context Warning
1 participant
Why is this change needed?
The
spp_alertsmodule had alert rules (spp.alert.rule) as configuration-only — nothing evaluated them to create alerts automatically. This made the module unusable standalone. Adding a generic rule evaluation engine allows alert rules to automatically monitor models and create alerts based on threshold and date conditions, making the module ready for Beta.How was the change implemented?
spp.alert.rule:rule_type(threshold/date),domain_filter,monitored_field_id,date_field_id,comparisonspp.alert:rule_id,res_model,res_id(all optional, no impact on existing alerts)_cron_evaluate_rules) and manual "Run Now" button on rule formdevelopment_statusfrom Alpha to BetaDRIMS compatibility verified: DRIMS uses prototype inheritance (
_name = "spp.drims.alert") with separate DB table and its own crons — completely unaffected.New unit tests
test_rule_evaluation.pycovering:Unit tests executed by the author
How to test manually
spp_alertsmoduleRelated links