feat(spp_data_classification): port classification registry from openspp-modules - #233
Merged
Merged
Conversation
![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 introduces the spp_data_classification module for OpenSPP, establishing a data sensitivity classification registry that maps model fields to sensitivity levels and provides regex/CEL auto-detection patterns. The review feedback highlights several critical performance optimizations and robustness improvements. Specifically, it recommends pre-fetching active patterns and existing classifications to eliminate redundant database queries during batch scanning, validating regular expressions on write, catching general exceptions during CEL evaluation to prevent scanner crashes, and correcting @api.depends paths for computed fields to prevent stale display names.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+312
to
+330
| @api.model | ||
| def find_matching_pattern(self, field_name, model_name=None, field_obj=None, model_obj=None): | ||
| """Find the first (highest priority) matching pattern. | ||
|
|
||
| Args: | ||
| field_name: The field name to check | ||
| model_name: Optional model name for scope filtering | ||
| field_obj: Optional ir.model.fields record for CEL evaluation | ||
| model_obj: Optional ir.model record for CEL evaluation | ||
|
|
||
| Returns: | ||
| recordset: The matching pattern or empty recordset | ||
| """ | ||
| patterns = self.search([("active", "=", True)]) | ||
| for pattern in patterns: | ||
| if pattern.matches_field(field_name, model_name, field_obj, model_obj): | ||
| return pattern | ||
| return self.browse() | ||
|
|
There was a problem hiding this comment.
Calling self.search([("active", "=", True)]) inside find_matching_pattern triggers a database query for every single field being checked. During a full registry scan (scan_all_models), this results in thousands of redundant SQL queries, creating a severe performance bottleneck. Allowing an optional pre-fetched patterns recordset parameter avoids this
Suggested change
| @api.model | |
| def find_matching_pattern(self, field_name, model_name=None, field_obj=None, model_obj=None): | |
| """Find the first (highest priority) matching pattern. | |
| Args: | |
| field_name: The field name to check | |
| model_name: Optional model name for scope filtering | |
| field_obj: Optional ir.model.fields record for CEL evaluation | |
| model_obj: Optional ir.model record for CEL evaluation | |
| Returns: | |
| recordset: The matching pattern or empty recordset | |
| """ | |
| patterns = self.search([("active", "=", True)]) | |
| for pattern in patterns: | |
| if pattern.matches_field(field_name, model_name, field_obj, model_obj): | |
| return pattern | |
| return self.browse() | |
| @api.model | |
| def find_matching_pattern(self, field_name, model_name=None, field_obj=None, model_obj=None, patterns=None): | |
| """Find the first (highest priority) matching pattern. | |
| Args: | |
| field_name: The field name to check | |
| model_name: Optional model name for scope filtering | |
| field_obj: Optional ir.model.fields record for CEL evaluation | |
| model_obj: Optional ir.model record for CEL evaluation | |
| patterns: Optional pre-fetched active patterns recordset | |
| Returns: | |
| recordset: The matching pattern or empty recordset | |
| """ | |
| if patterns is None: | |
| patterns = self.search([("active", "=", True)]) | |
| for pattern in patterns: | |
| if pattern.matches_field(field_name, model_name, field_obj, model_obj): | |
| return pattern | |
| return self.browse() |
Comment on lines
+331
to
+366
| @api.model | ||
| def auto_classify_field(self, model_name, field_name, field_obj=None, model_obj=None): | ||
| """Attempt to auto-classify a field based on patterns. | ||
|
|
||
| Args: | ||
| model_name: The model name | ||
| field_name: The field name | ||
| field_obj: Optional ir.model.fields record for CEL evaluation | ||
| model_obj: Optional ir.model record for CEL evaluation | ||
|
|
||
| Returns: | ||
| recordset: The created classification or empty if no match | ||
| """ | ||
| pattern = self.find_matching_pattern(field_name, model_name, field_obj, model_obj) | ||
| if not pattern: | ||
| return self.env["spp.field.classification"].browse() | ||
|
|
||
| _logger.info( | ||
| "Auto-classifying %s.%s as %s (pattern: %s, mode: %s)", | ||
| model_name, | ||
| field_name, | ||
| pattern.classification_id.code, | ||
| pattern.name, | ||
| pattern.match_mode, | ||
| ) | ||
|
|
||
| return self.env["spp.field.classification"].ensure_classification( | ||
| model_name=model_name, | ||
| field_name=field_name, | ||
| level_code=pattern.classification_id.code, | ||
| source="auto", | ||
| pattern_id=pattern.id, | ||
| pii_category=pattern.pii_category, | ||
| mask_pattern=pattern.default_mask_pattern, | ||
| search_strategy=pattern.default_search_strategy, | ||
| ) |
There was a problem hiding this comment.
Pass the optional pre-fetched patterns recordset to find_matching_pattern to avoid redundant database queries during batch scanning.
@api.model
def auto_classify_field(self, model_name, field_name, field_obj=None, model_obj=None, patterns=None):
"""Attempt to auto-classify a field based on patterns.
Args:
model_name: The model name
field_name: The field name
field_obj: Optional ir.model.fields record for CEL evaluation
model_obj: Optional ir.model record for CEL evaluation
patterns: Optional pre-fetched active patterns recordset
Returns:
recordset: The created classification or empty if no match
"""
pattern = self.find_matching_pattern(field_name, model_name, field_obj, model_obj, patterns=patterns)
if not pattern:
return self.env["spp.field.classification"].browse()
_logger.info(
"Auto-classifying %s.%s as %s (pattern: %s, mode: %s)",
model_name,
field_name,
pattern.classification_id.code,
pattern.name,
pattern.match_mode,
)
return self.env["spp.field.classification"].ensure_classification(
model_name=model_name,
field_name=field_name,
level_code=pattern.classification_id.code,
source="auto",
pattern_id=pattern.id,
pii_category=pattern.pii_category,
mask_pattern=pattern.default_mask_pattern,
search_strategy=pattern.default_search_strategy,
)
Comment on lines
+369
to
+401
| def scan_model_fields(self, model_name, skip_classified=True): | ||
| """Scan all fields of a model and auto-classify matches. | ||
|
|
||
| Args: | ||
| model_name: The model to scan | ||
| skip_classified: Skip fields that already have classifications | ||
|
|
||
| Returns: | ||
| list: List of (field_name, classification) tuples | ||
| """ | ||
| model = self.env["ir.model"].search([("model", "=", model_name)], limit=1) | ||
| if not model: | ||
| _logger.warning("Model not found: %s", model_name) | ||
| return [] | ||
|
|
||
| results = [] | ||
| for field in model.field_id: | ||
| # Skip non-data fields | ||
| if field.ttype in ("one2many", "many2many"): | ||
| continue | ||
|
|
||
| # Skip if already classified | ||
| if skip_classified: | ||
| existing = self.env["spp.field.classification"].get_classification(model_name, field.name) | ||
| if existing: | ||
| continue | ||
|
|
||
| # Try to auto-classify (pass field and model objects for CEL evaluation) | ||
| classification = self.auto_classify_field(model_name, field.name, field, model) | ||
| if classification: | ||
| results.append((field.name, classification)) | ||
|
|
||
| return results |
There was a problem hiding this comment.
Currently, scan_model_fields performs a database query get_classification for every single field to check if it is already classified. For models with many fields, this results in
Note: The search on ir.model with limit=1 and no order is non-deterministic, but acceptable here as model is unique in this context. This should be acknowledged as technical debt to be addressed if the context changes.
@api.model
def scan_model_fields(self, model_name, skip_classified=True, patterns=None):
"""Scan all fields of a model and auto-classify matches.
Args:
model_name: The model to scan
skip_classified: Skip fields that already have classifications
patterns: Optional pre-fetched active patterns recordset
Returns:
list: List of (field_name, classification) tuples
"""
model = self.env["ir.model"].search([("model", "=", model_name)], limit=1)
if not model:
_logger.warning("Model not found: %s", model_name)
return []
if patterns is None:
patterns = self.search([("active", "=", True)])
classified_fields = set()
if skip_classified:
classifications = self.env["spp.field.classification"].get_model_classifications(model_name)
classified_fields = set(classifications.mapped("field_name"))
results = []
for field in model.field_id:
# Skip non-data fields
if field.ttype in ("one2many", "many2many"):
continue
# Skip if already classified
if skip_classified and field.name in classified_fields:
continue
# Try to auto-classify (pass field and model objects for CEL evaluation)
classification = self.auto_classify_field(model_name, field.name, field, model, patterns=patterns)
if classification:
results.append((field.name, classification))
return resultsReferences
- A non-deterministic database search (using
searchwithlimit=1but noorder) can be acceptable if the practical context makes it functionally deterministic. However, this should be acknowledged as technical debt to be addressed if the context changes.
Comment on lines
+404
to
+428
| def scan_all_models(self, model_pattern=None, skip_classified=True): | ||
| """Scan all models (or matching pattern) for PII fields. | ||
|
|
||
| Args: | ||
| model_pattern: Optional regex pattern to filter models | ||
| skip_classified: Skip fields that already have classifications | ||
|
|
||
| Returns: | ||
| dict: {model_name: [(field_name, classification), ...]} | ||
| """ | ||
| domain = [("transient", "=", False)] | ||
| models = self.env["ir.model"].search(domain) | ||
|
|
||
| if model_pattern: | ||
| pattern = self._get_compiled_pattern(model_pattern) | ||
| if pattern: | ||
| models = models.filtered(lambda m: pattern.search(m.model)) | ||
|
|
||
| results = {} | ||
| for model in models: | ||
| model_results = self.scan_model_fields(model.model, skip_classified) | ||
| if model_results: | ||
| results[model.model] = model_results | ||
|
|
||
| return results |
There was a problem hiding this comment.
Pre-fetch the active patterns once at the start of scan_all_models and pass them down to scan_model_fields to avoid querying the database for patterns on every single model and field.
@api.model
def scan_all_models(self, model_pattern=None, skip_classified=True):
"""Scan all models (or matching pattern) for PII fields.
Args:
model_pattern: Optional regex pattern to filter models
skip_classified: Skip fields that already have classifications
Returns:
dict: {model_name: [(field_name, classification), ...]}
"""
domain = [("transient", "=", False)]
models = self.env["ir.model"].search(domain)
if model_pattern:
pattern = self._get_compiled_pattern(model_pattern)
if pattern:
models = models.filtered(lambda m: pattern.search(m.model))
patterns = self.search([("active", "=", True)])
results = {}
for model in models:
model_results = self.scan_model_fields(model.model, skip_classified, patterns=patterns)
if model_results:
results[model.model] = model_results
return results
Comment on lines
+145
to
+154
| @api.constrains("match_mode", "pattern", "cel_expression") | ||
| def _check_pattern_or_expression(self): | ||
| """Ensure either pattern or CEL expression is provided based on mode.""" | ||
| for record in self: | ||
| if record.match_mode == "regex": | ||
| if not record.pattern: | ||
| raise ValidationError(_("Regex pattern is required when using Regex match mode")) | ||
| elif record.match_mode == "cel": | ||
| if not record.cel_expression: | ||
| raise ValidationError(_("CEL expression is required when using CEL match mode")) |
There was a problem hiding this comment.
It is highly recommended to validate the regular expressions (pattern and apply_to_model_pattern) when saving the record. Currently, invalid regex patterns can be saved without any validation, which will lead to runtime warnings and potential bypasses of model filters. Adding validation in the @api.constrains method ensures robustness.
Suggested change
| @api.constrains("match_mode", "pattern", "cel_expression") | |
| def _check_pattern_or_expression(self): | |
| """Ensure either pattern or CEL expression is provided based on mode.""" | |
| for record in self: | |
| if record.match_mode == "regex": | |
| if not record.pattern: | |
| raise ValidationError(_("Regex pattern is required when using Regex match mode")) | |
| elif record.match_mode == "cel": | |
| if not record.cel_expression: | |
| raise ValidationError(_("CEL expression is required when using CEL match mode")) | |
| @api.constrains("match_mode", "pattern", "cel_expression", "apply_to_model_pattern") | |
| def _check_pattern_or_expression(self): | |
| """Ensure either pattern or CEL expression is provided based on mode and validate regexes.""" | |
| for record in self: | |
| if record.match_mode == "regex": | |
| if not record.pattern: | |
| raise ValidationError(_("Regex pattern is required when using Regex match mode")) | |
| try: | |
| re.compile(record.pattern) | |
| except re.error as e: | |
| raise ValidationError(_("Invalid regex pattern '%s': %s") % (record.pattern, e)) | |
| elif record.match_mode == "cel": | |
| if not record.cel_expression: | |
| raise ValidationError(_("CEL expression is required when using CEL match mode")) | |
| if record.apply_to_model_pattern: | |
| try: | |
| re.compile(record.apply_to_model_pattern) | |
| except re.error as e: | |
| raise ValidationError(_("Invalid model pattern regex '%s': %s") % (record.apply_to_model_pattern, e)) |
Comment on lines
+237
to
+253
| try: | ||
| result = cel_service.evaluate_expression(self.cel_expression, context) | ||
| return bool(result) | ||
| except (ValueError, TypeError, KeyError, AttributeError) as e: | ||
| _logger.warning( | ||
| "CEL evaluation failed for pattern '%s': %s", | ||
| self.name, | ||
| str(e), | ||
| ) | ||
| return False | ||
| except SyntaxError as e: | ||
| _logger.warning( | ||
| "CEL syntax error in pattern '%s': %s", | ||
| self.name, | ||
| str(e), | ||
| ) | ||
| return False |
There was a problem hiding this comment.
Catching only specific exceptions during CEL evaluation can lead to unhandled exceptions (e.g., custom exceptions from the CEL service or other runtime errors) crashing the entire scan process. It is safer to catch Exception as a fallback to ensure the scanner is robust and continues scanning other fields even if one pattern has an evaluation error.
Suggested change
| try: | |
| result = cel_service.evaluate_expression(self.cel_expression, context) | |
| return bool(result) | |
| except (ValueError, TypeError, KeyError, AttributeError) as e: | |
| _logger.warning( | |
| "CEL evaluation failed for pattern '%s': %s", | |
| self.name, | |
| str(e), | |
| ) | |
| return False | |
| except SyntaxError as e: | |
| _logger.warning( | |
| "CEL syntax error in pattern '%s': %s", | |
| self.name, | |
| str(e), | |
| ) | |
| return False | |
| try: | |
| result = cel_service.evaluate_expression(self.cel_expression, context) | |
| return bool(result) | |
| except Exception as e: | |
| _logger.warning( | |
| "CEL evaluation failed for pattern '%s': %s", | |
| self.name, | |
| str(e), | |
| ) | |
| return False |
Comment on lines
+179
to
+188
| @api.depends("model_id", "field_id", "classification_id") | ||
| def _compute_display_name(self): | ||
| for record in self: | ||
| if record.model_id and record.field_id: | ||
| record.display_name = ( | ||
| f"{record.model_id.model}.{record.field_id.name} [{record.classification_id.code or 'N/A'}]" | ||
| ) | ||
| else: | ||
| record.display_name = _("New Classification") | ||
|
|
There was a problem hiding this comment.
The computed field display_name depends on model_id.model, field_id.name, and classification_id.code. Currently, the @api.depends decorator only specifies model_id, field_id, and classification_id. If the code of a classification level changes, or if a model/field name is updated, the display name will not be recomputed and will become stale. Specifying the full paths in @api.depends fixes this issue.
Suggested change
| @api.depends("model_id", "field_id", "classification_id") | |
| def _compute_display_name(self): | |
| for record in self: | |
| if record.model_id and record.field_id: | |
| record.display_name = ( | |
| f"{record.model_id.model}.{record.field_id.name} [{record.classification_id.code or 'N/A'}]" | |
| ) | |
| else: | |
| record.display_name = _("New Classification") | |
| @api.depends("model_id.model", "field_id.name", "classification_id.code") | |
| def _compute_display_name(self): | |
| for record in self: | |
| if record.model_id and record.field_id: | |
| record.display_name = ( | |
| f"{record.model_id.model}.{record.field_id.name} [{record.classification_id.code or 'N/A'}]" | |
| ) | |
| else: | |
| record.display_name = _("New Classification") |
Comment on lines
+132
to
+144
| def is_more_sensitive_than(self, other_level): | ||
| """Compare sensitivity between two levels. | ||
|
|
||
| Args: | ||
| other_level: Another classification level to compare against | ||
|
|
||
| Returns: | ||
| bool: True if this level is more sensitive (higher sequence) | ||
| """ | ||
| self.ensure_one() | ||
| if not other_level: | ||
| return True | ||
| return self.sequence > other_level.sequence |
There was a problem hiding this comment.
To prevent potential runtime errors if other_level is a recordset containing multiple records, it is safer to explicitly call ensure_one() on it before accessing its attributes.
Suggested change
| def is_more_sensitive_than(self, other_level): | |
| """Compare sensitivity between two levels. | |
| Args: | |
| other_level: Another classification level to compare against | |
| Returns: | |
| bool: True if this level is more sensitive (higher sequence) | |
| """ | |
| self.ensure_one() | |
| if not other_level: | |
| return True | |
| return self.sequence > other_level.sequence | |
| def is_more_sensitive_than(self, other_level): | |
| """Compare sensitivity between two levels. | |
| Args: | |
| other_level: Another classification level to compare against | |
| Returns: | |
| bool: True if this level is more sensitive (higher sequence) | |
| """ | |
| self.ensure_one() | |
| if not other_level: | |
| return True | |
| other_level.ensure_one() | |
| return self.sequence > other_level.sequence |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #233 +/- ##
==========================================
+ Coverage 76.59% 76.72% +0.12%
==========================================
Files 629 634 +5
Lines 42351 42625 +274
==========================================
+ Hits 32439 32703 +264
- Misses 9912 9922 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gonzalesedwin1123
marked this pull request as draft
June 11, 2026 01:42
…spp-modules Migrate the data-classification foundation to OpenSPP2 (Odoo 19), scoped to the classification registry that downstream encryption/audit consumers need: - spp.data.classification.level: sensitivity levels (PUBLIC..RESTRICTED) with policy flags - spp.field.classification: maps model fields to levels + PII category; adds is_pii (computed-stored from pii_category) so consumers can query PII fields - spp.classification.pattern: regex/CEL auto-detection patterns (CEL degrades gracefully when spp.cel.service is absent) Depends on base, spp_security. Seeds 4 levels + 15 regex detection patterns. Deferred to a separate governance PR (they patch core ORM / carry untested surface): pii_aware_mixin (base.read() masking + Field monkeypatch), DSAR, data retention + cron, and consent integration. Realises ADR-011. Verified: 19/19 tests pass (incl. new is_pii test), module installs, pre-commit (compliance hooks + bandit + semgrep) green.
…ystem implied_ids grants the implied groups to members, so the ported link silently promoted anyone holding Data Classification Admin to a full Settings/System administrator (same escalation removed from spp_dci and spp_key_management, and explicitly warned against in spp_pii_encryption). The admin group now implies the manager group instead (keeping the menus, which are gated on manager, visible to admins), and system administrators get explicit ACL rows on the three registry models, mirroring spp_pii_encryption. Regression tests pin the group topology and the admin ACLs.
… gaps - Validate regexes (pattern, model scope) at save time via api.constrains; previously an uncompilable regex saved fine and just never matched, logging a warning only at scan time. - Catch any evaluation error in _matches_field_cel, not just the narrow tuple: spp.cel.service re-raises whatever the CEL parser/evaluator throws (incl. RecursionError), so one broken pattern could crash an entire registry scan. - Recompute the stored display_name when the level code or the underlying model/field technical name changes (was stale before). - Give model_name an explicit "Model Name" label; the related field inherited ir.model's "Model" string and clashed with model_id's label, making Odoo warn on every registry load. - Cover the CEL degradation/success/error paths, registry scans, the Test Pattern action, and ensure_classification fallbacks with tests (codecov patch was 65.28% vs the 70% gate, with classification_pattern at 45.27%).
gonzalesedwin1123
force-pushed
the
migrate-spp-data-classification
branch
from
August 25, 2026 04:22
2d31ab5 to
b057587
Compare
Applied verbatim from the CI pre-commit run (32808690341); the pinned CI generator is the sole authority for generated files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Migrates the data-classification foundation from
openspp-modules(v2) into OpenSPP2 (Odoo 19). Scoped to the classification registry — the part downstream encryption/audit consumers need. Realises ADR-011.This is PR2 in a sequence:
spp_pii_encryptioncorespp_data_classificationregistryspp.field.classification+is_pii) and wire themasked_charwidget into a real viewWhat's included
spp.data.classification.levelspp.field.classificationis_pii(computed-stored frompii_category) so consumers can query PII fieldsspp.classification.patternspp.cel.serviceis absent)Seeds 4 classification levels + 15 regex detection patterns. Depends on:
base,spp_security.What's intentionally deferred (to a separate governance PR)
These carry higher risk (core-ORM patching) or untested surface and aren't needed by the encryption consumers, so they're held back for a focused, well-tested change:
pii_aware_mixin(overridesbase.read()for auto-masking) +fields_extension(monkeypatchesfields.Field.__init__to acceptpii=/classification=kwargs). Wide blast radius across all ~117 modules; deserves its own PR.spp.dsar.request/.export), data retention (+ daily cron), consent integration (spp_consentbridge — never executed in openspp-modules, needs fresh testing incl. thepii_category→DPV-code mapping).Notable migration fix
is_piiwas queried but never defined in the source (consent_integration.py, and PR3's wizard filter[("is_pii","=",True)]) — latent dead code in openspp-modules wherespp_consentdidn't exist. Added it as a stored-computedbool(pii_category)so both the (future) consent code and PR3's wizard work, with a regression test.Adaptations
category→OpenSPP/Configuration,website→OpenSPP2,maintainers→["jeremi","gonzalesedwin1123"], deps trimmed (droppedmail,spp_consent),datalist trimmed.data_protection_officergroup (only DSAR/retention used it); kept the 4 groups referenced by retained data/ACLs.Verification
./spp t spp_data_classification→ 36/36 tests pass, module installs (re-verified after rebasing onto current 19.0)Notes
_setup_completeauto-trigger lived in the deferredpii_aware/fields_extension);scan_model_fields()is callable on demand.string=params; one positional-translation) were carried over unchanged from the source module.Review fixes (2026-08-25)
Rebased onto current 19.0 and addressed review findings:
group_classification_adminno longer impliesbase.group_system— the ported link silently promoted classification admins to full system administrators (the same escalation removed fromspp_dci/spp_key_management, and warned against inspp_pii_encryption). The admin group now implies the manager group (keeping the manager-gated menus visible), and system admins get explicit ACL rows on the three models instead. Regression tests pin the group topology.classification_pattern.pyat 45.27%). Added tests for the CEL degradation/success/error paths, registry scans, the Test Pattern action, andensure_classificationfallbacks.api.constrains); a broken CEL pattern can no longer crash a whole scan (broad catch, warn, no-match); storeddisplay_namerecomputes when the level code or field/model technical name changes;model_namegot an explicit label (was clashing withmodel_id's "Model" and warned on every registry load).