feat(spp_pii_encryption): port PII encryption core from openspp-modules - #232
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_pii_encryption module for Odoo, which implements field-level AES-256-GCM encryption for PII data, searchable blind indexes, UI-based configuration, and access audit logging. The review feedback highlights several critical and high-severity issues: a crash on module load due to the incorrect use of models.Constraint instead of _sql_constraints, stale blind indexes left in the database when encrypted fields are cleared during creation or modification, timezone inconsistencies from using datetime.now() instead of fields.Datetime.now(), and a misleading masking behavior for short values in the frontend widget.
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.
| _unique_model_field = models.Constraint( | ||
| "UNIQUE(model_id, field_id)", | ||
| "Encryption configuration already exists for this model and field!", | ||
| ) |
There was a problem hiding this comment.
Odoo does not have a models.Constraint class. Defining a constraint this way will raise an AttributeError on module load, preventing the module from starting or installing. SQL constraints in Odoo must be defined using the _sql_constraints list of tuples.
_sql_constraints = [
(
"unique_model_field",
"UNIQUE(model_id, field_id)",
"Encryption configuration already exists for this model and field!",
)
]There was a problem hiding this comment.
Not applied — models.Constraint is the correct form on Odoo 19 (see e.g. spp_registry/models/reg_id.py); _sql_constraints is the one that is ignored there. The module installs and all 25 tests pass with this constraint in place.
| def write(self, vals): | ||
| """Encrypt fields before write.""" | ||
| encrypted_fields = self._get_encrypted_fields() | ||
| if not encrypted_fields: | ||
| return super().write(vals) | ||
|
|
||
| for field_name in encrypted_fields: | ||
| if field_name in vals and vals[field_name]: | ||
| plaintext = vals[field_name] | ||
|
|
||
| # Encrypt | ||
| vals[field_name] = self._encrypt_value(plaintext, field_name) | ||
|
|
||
| # Get configured index type | ||
| index_type = self._get_index_type(field_name) | ||
|
|
||
| # Compute blind indexes if fields exist | ||
| index_field = f"{field_name}_index" | ||
| if index_field in self._fields: | ||
| vals[index_field] = self._compute_blind_index(plaintext, field_name, index_type) | ||
|
|
||
| last4_field = f"{field_name}_last4" | ||
| if last4_field in self._fields: | ||
| # SECURITY: Store hashed partial index, not plaintext | ||
| vals[last4_field] = self._compute_blind_index(plaintext, field_name, "partial") | ||
|
|
||
| return super().write(vals) |
There was a problem hiding this comment.
If an encrypted field is cleared (set to False or ""), the current implementation skips the encryption block entirely and does not update the corresponding blind index fields (_index and _last4). This leaves stale hashes in the database, allowing searches to find records that have actually had their PII cleared. We should explicitly clear the index fields when the main field is cleared.
def write(self, vals):
"""Encrypt fields before write."""
encrypted_fields = self._get_encrypted_fields()
if not encrypted_fields:
return super().write(vals)
for field_name in encrypted_fields:
if field_name in vals:
plaintext = vals[field_name]
if plaintext:
# Encrypt
vals[field_name] = self._encrypt_value(plaintext, field_name)
# Get configured index type
index_type = self._get_index_type(field_name)
# Compute blind indexes if fields exist
index_field = f"{field_name}_index"
if index_field in self._fields:
vals[index_field] = self._compute_blind_index(plaintext, field_name, index_type)
last4_field = f"{field_name}_last4"
if last4_field in self._fields:
# SECURITY: Store hashed partial index, not plaintext
vals[last4_field] = self._compute_blind_index(plaintext, field_name, "partial")
else:
# Clear indexes if the field is cleared
index_field = f"{field_name}_index"
if index_field in self._fields:
vals[index_field] = False
last4_field = f"{field_name}_last4"
if last4_field in self._fields:
vals[last4_field] = False
return super().write(vals)There was a problem hiding this comment.
Fixed in e0eb689: create()/write() now share _apply_encryption_to_vals(), which clears *_index/*_last4 when the encrypted field is cleared. Covered by test_apply_encryption_to_vals_clears_stale_indexes.
| @api.model_create_multi | ||
| def create(self, vals_list): | ||
| """Encrypt fields before create.""" | ||
| encrypted_fields = self._get_encrypted_fields() | ||
| if not encrypted_fields: | ||
| return super().create(vals_list) | ||
|
|
||
| for vals in vals_list: | ||
| for field_name in encrypted_fields: | ||
| if field_name in vals and vals[field_name]: | ||
| plaintext = vals[field_name] | ||
|
|
||
| # Encrypt | ||
| vals[field_name] = self._encrypt_value(plaintext, field_name) | ||
|
|
||
| # Get configured index type | ||
| index_type = self._get_index_type(field_name) | ||
|
|
||
| # Compute blind indexes if fields exist | ||
| index_field = f"{field_name}_index" | ||
| if index_field in self._fields: | ||
| vals[index_field] = self._compute_blind_index(plaintext, field_name, index_type) | ||
|
|
||
| last4_field = f"{field_name}_last4" | ||
| if last4_field in self._fields: | ||
| # SECURITY: Store hashed partial index, not plaintext | ||
| vals[last4_field] = self._compute_blind_index(plaintext, field_name, "partial") | ||
|
|
||
| return super().create(vals_list) |
There was a problem hiding this comment.
For consistency and safety, we should also handle the clearing of index fields during record creation if the field is explicitly passed as falsy or empty.
@api.model_create_multi
def create(self, vals_list):
"""Encrypt fields before create."""
encrypted_fields = self._get_encrypted_fields()
if not encrypted_fields:
return super().create(vals_list)
for vals in vals_list:
for field_name in encrypted_fields:
if field_name in vals:
plaintext = vals[field_name]
if plaintext:
# Encrypt
vals[field_name] = self._encrypt_value(plaintext, field_name)
# Get configured index type
index_type = self._get_index_type(field_name)
# Compute blind indexes if fields exist
index_field = f"{field_name}_index"
if index_field in self._fields:
vals[index_field] = self._compute_blind_index(plaintext, field_name, index_type)
last4_field = f"{field_name}_last4"
if last4_field in self._fields:
# SECURITY: Store hashed partial index, not plaintext
vals[last4_field] = self._compute_blind_index(plaintext, field_name, "partial")
else:
index_field = f"{field_name}_index"
if index_field in self._fields:
vals[index_field] = False
last4_field = f"{field_name}_last4"
if last4_field in self._fields:
vals[last4_field] = False
return super().create(vals_list)There was a problem hiding this comment.
Fixed in e0eb689 — same shared helper handles create and write uniformly, including explicit falsy values on create.
| from datetime import datetime, timedelta | ||
|
|
||
| if user_id is None: | ||
| user_id = self.env.user.id | ||
|
|
||
| cutoff = datetime.now() - timedelta(days=days) |
There was a problem hiding this comment.
Using datetime.now() retrieves the server's local time, which can lead to timezone inconsistencies and incorrect query results because Odoo stores datetime values in UTC in the database. Instead, use fields.Datetime.now() to get the current UTC datetime.
| from datetime import datetime, timedelta | |
| if user_id is None: | |
| user_id = self.env.user.id | |
| cutoff = datetime.now() - timedelta(days=days) | |
| from datetime import timedelta | |
| if user_id is None: | |
| user_id = self.env.user.id | |
| cutoff = fields.Datetime.now() - timedelta(days=days) |
There was a problem hiding this comment.
Fixed in b2ab590: uses fields.Datetime.now() (UTC) and passes the datetime directly to the domain. Window behaviour covered by test_get_user_access_history_window_excludes_old_entries.
| applyMask(value, pattern) { | ||
| if (!value || !pattern) return "****"; | ||
|
|
||
| // Pattern interpretation: | ||
| // * = mask this character | ||
| // # = show this character from the end | ||
| // Other chars = literal | ||
|
|
||
| const valueChars = value.split(""); | ||
| const patternChars = pattern.split(""); | ||
| let result = ""; | ||
| let valueIdx = 0; | ||
| const endIdx = value.length - 1; | ||
|
|
||
| // Count # in pattern to know how many chars to show from end | ||
| const hashCount = patternChars.filter((c) => c === "#").length; | ||
| let showFromEnd = hashCount; | ||
|
|
||
| for (const patternChar of patternChars) { | ||
| if (patternChar === "*") { | ||
| // Mask character | ||
| result += "•"; | ||
| valueIdx++; | ||
| } else if (patternChar === "#") { | ||
| // Show character from end | ||
| const showIdx = value.length - showFromEnd; | ||
| if (showIdx >= 0 && showIdx < value.length) { | ||
| result += value[showIdx]; | ||
| } | ||
| showFromEnd--; | ||
| } else { | ||
| // Literal character (like - or space) | ||
| result += patternChar; | ||
| } | ||
| } | ||
|
|
||
| return result || "••••••••"; | ||
| } |
There was a problem hiding this comment.
The current applyMask implementation pads the output with fake bullets to match the full pattern length even if the actual value is much shorter (e.g., a 3-character value like "123" gets masked as "••••-••••-123"). This is highly misleading as it makes short values look long. Additionally, valueIdx is incremented but never used. We should align the value with the pattern from right to left to respect the actual length of the value.
applyMask(value, pattern) {
if (!value || !pattern) return "****";
const valueChars = value.split("");
const patternChars = pattern.split("");
const resultChars = [];
let valIdx = valueChars.length - 1;
for (let i = patternChars.length - 1; i >= 0; i--) {
if (valIdx < 0) break;
const pChar = patternChars[i];
if (pChar === "#") {
resultChars.unshift(valueChars[valIdx]);
valIdx--;
} else if (pChar === "*") {
resultChars.unshift("•");
valIdx--;
} else {
resultChars.unshift(pChar);
if (valueChars[valIdx] === pChar) {
valIdx--;
}
}
}
while (valIdx >= 0) {
resultChars.unshift("•");
valIdx--;
}
return resultChars.join("") || "••••••••";
}There was a problem hiding this comment.
Partially applied in 92fce95. The pattern-length (rather than value-length) mask is deliberate: it avoids leaking how long the real value is, which matters for PII. The real leak here was short values: a value with length <= the number of # slots was displayed in full. That is fixed — such values are now fully masked. The unused valueIdx no longer exists in the current implementation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #232 +/- ##
==========================================
+ Coverage 75.76% 75.95% +0.18%
==========================================
Files 546 552 +6
Lines 36576 36906 +330
==========================================
+ Hits 27713 28031 +318
- Misses 8863 8875 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Migrate the field-level PII encryption module to OpenSPP2 (Odoo 19): - spp.encrypted.field.mixin: transparent AES-256-GCM encryption with HMAC-SHA256 blind indexes (exact/partial/phonetic) for searchability - spp.field.encryption.config: UI-based per-field encryption config - spp.pii.audit.log: audit trail of PII field access - masked_char OWL widget for masked display with reveal + audit Integrates with spp_key_management's key manager (get_key/get_salt). Depends on base, spp_key_management, spp_registry, spp_security. The bulk-migration wizard is intentionally deferred to a follow-up PR: it depends on spp_data_classification, which is not yet migrated to OpenSPP2. Realises ADR-011/ADR-012. Verified: 7/7 mixin tests pass, module installs in full UI stack, backend asset bundle compiles with the masked widget included.
- masked_field.js: remove unused vars (Component import, valueChars, endIdx, valueIdx, unused catch binding) flagged by eslint no-unused-vars - regenerate README.rst / index.html to match the manifest maintainers (oca-gen-addon-readme was failing on stale maintainer block)
…lpers Raise patch coverage on the migrated module: - test_field_encryption_config: get_encrypted_fields/get_field_config/ is_field_encrypted/get_index_type, index-field-name computation (index vs last4), toggle actions, and the char/text-only constraint - test_audit_log: log_field_access, get_access_history, get_user_access_history, display_name - test_encrypted_field_mixin: _get_encrypted_fields default, _get_index_type default, phonetic/passthrough normalization, graceful decrypt failure, and search helpers when no index column exists Note: the mixin's create/write/read ORM hooks can't be unit-tested in isolation (a tests/-defined concrete model isn't registered in this harness, same limitation spp_approval skips around). They are exercised by consumer modules that apply the mixin to real PII fields.
…ind indexes Two mixin fixes found in review + rigid testing: - env.get() returns an empty (falsy) recordset, so 'if FieldConfig:' never passed and the spp.field.encryption.config table was never consulted: the UI-based configuration was completely inert (bug also present in the openspp-modules source module). Compare against None instead. - Clearing an encrypted field left its *_index/*_last4 blind indexes in place, so the stale HMAC hashes kept matching searches after the PII was removed. create()/write() now share _apply_encryption_to_vals(), which clears the indexes when the value is cleared. Adds helper unit tests and a config-driven write() integration test.
create_date is stored in UTC; datetime.now() is server-local, skewing the get_user_access_history day window on non-UTC servers. Use fields.Datetime.now() and pass the datetime directly to the domain.
…ryption admin group implied_ids GRANTS the implied group, so linking base.group_system would silently promote any Encryption Administrator to a full Settings/System admin - the same pattern removed from spp_dci and spp_key_management in #399. Imply base.group_user only; system admins get an explicit ACL row on the config model instead.
useService('rpc') and useService('user') no longer exist in Odoo 19, so
setup() would crash as soon as a view used the widget. Use the orm service
for the audit call, the user object from @web/core/user for the group
check, and _t() for user-facing strings. Also stop revealing the tail of a
value shorter than or equal to the number of '#' slots - the whole value
would have been displayed; the mask stays pattern-length by design so it
does not leak value length.
spp_registry was referenced only in docstrings; the module is a library layer over base, spp_key_management and spp_security.
c030ea6 to
67bf0ee
Compare
…lure paths Codecov flagged 33 uncovered lines in encrypted_field_mixin.py - the create()/read() overrides, both blind-index search helpers, the encryption-failure branch and the Soundex empty guard, all paths that normally need a concrete inheriting model. Cover them by mocking the super() ORM boundary (BaseModel.create/read, the mixin's search) around the real mixin logic, with key/salt records materialized outside the patch scopes. 32 tests total.
- rename read() param to 'fields' to match the Odoo 19 core signature (core callers like res.users use read(fields=...) by keyword) - sudo() the encryption-config lookups: it is a policy table and portal reads of a host model must not crash on its ACL - make the blind-index search helpers private - as public RPC methods they were a plaintext-confirmation / last-4-bucketing oracle - re-raise AccessError from _encrypt_value (missing key permission is a permission problem) and use UserError for genuine crypto failures - drop the dead _encryption_enabled field: nothing read it and it would materialize an unexplained column on every adopting model
The method is RPC-reachable and creates rows with sudo(), so any authenticated user (portal included) could flood or forge the audit trail with entries pointing at arbitrary model/record/field strings. Now the model and field must exist, the record must exist, and the caller must be able to read it. Rate limiting and retention are follow-ups.
… config Odoo silently truncates Char values to their size limit; a truncated ciphertext can never be decrypted, so encrypting such a field destroys the plaintext on the first write with no error. Translated fields store per-language jsonb values and would fragment the ciphertext.
… docs - admit base.group_system to the PII Encryption menu: nobody holds group_encryption_admin on a fresh install, so the UI was unreachable (menu visibility only - no implied_ids escalation) - drop the base.group_user read ACL on the config model: internal users could enumerate the PII field map, and the mixin no longer needs it since its config lookup is sudo'd - DESCRIPTION.md: correct the dependency list, menu paths, audit-scope claim, and document the private search helpers
Applied verbatim from the pre-commit run's --show-diff-on-failure output (run 32802395229); the local generator is not byte-reproducible against CI's pinned hook env.
1 participant
What
Migrates the field-level PII encryption module from
openspp-modulesto OpenSPP2 (Odoo 19). This is PR1 of a planned sequence and ports the runtime encryption core; the bulk-migration wizard is deferred (see below).Realises ADR-011 (Data Classification) / ADR-012 (PII Encryption Strategy).
What's included
spp.encrypted.field.mixinspp.field.encryption.configspp.pii.audit.logmasked_char(OWL)Integrates with
spp_key_management's key manager (get_key/get_salt).Depends on:
base,spp_key_management,spp_security. External python:cryptography.What's intentionally deferred
The bulk-migration wizard (scan / dry-run / migrate / backup / rollback of existing plaintext) is not in this PR. Its scan step depends on
spp.field.classificationfromspp_data_classification, which is not yet migrated to OpenSPP2. Plan:spp_data_classificationAdaptations from the source module
category→OpenSPP/Configuration,website→ OpenSPP2,auto_install: False(admin opt-in tool), dependency list trimmed (droppedspp_data_classification, the unusedspp_encryption, and the unusedspp_registry— it was referenced only in docstrings).ValidationErrormessage in_()for translation.Fixes applied on review (2026-08-25, after rebase onto current 19.0)
group_encryption_adminno longer impliesbase.group_system(the same privilege-escalation pattern removed fromspp_dci/spp_key_managementin security: batch 1 — DCI/Key admin privilege escalations, OAuth signing keys, GRM rule ACL (#327, #329, #265, #266) #399). It now impliesbase.group_useronly; system admins get explicit ACL rows instead.*_index/*_last4blind indexes — previously the stale HMAC hashes stayed searchable after the PII was removed (Gemini finding).create()/write()now share one_apply_encryption_to_vals()helper.masked_charwidget ported to Odoo 19 web APIs —useService("rpc")anduseService("user")no longer exist in Odoo 19 (setup()would crash as soon as a view used the widget). Now uses theormservice,userfrom@web/core/user, and_tfor user-facing strings.applyMaskno longer reveals the tail of a value shorter than (or equal to) the number of#slots — a short value would otherwise have been displayed in full. The mask output deliberately stays pattern-length (does not leak value length).get_user_access_historyusesfields.Datetime.now()(UTC) instead of server-localdatetime.now()(Gemini finding).self.env.get("spp.field.encryption.config")returns an empty recordset, which is falsy, soif FieldConfig:never passed and_get_encrypted_fields()/_get_index_type()never consultedspp.field.encryption.configat all. The UI-based configuration (this module's headline mechanism) could never take effect. Now compares againstNone. The same bug exists in the source module inopenspp-modules(encrypted_field_mixin.py:82,133) — the feature has been inert upstream too.Not applied: Gemini's "critical" claiming
models.Constraintdoesn't exist — on Odoo 19models.Constraintis the correct form (_sql_constraintsis the one that's ignored); the module installs and all tests pass.Second fix round (adversarial staff-engineer review, 2026-08-25)
An adversarial review pass found 8 further must-fixes, all applied:
read()override usedfields_list=as the parameter name; Odoo 19 core callsread(fields=...)by keyword (e.g.res.users,mail.message), which wouldTypeErroron the first adopter with such an ancestor. Renamed to match core.group_encryption_adminand no user holds that group out of the box. The menu now also admitsbase.group_system(menu visibility only; noimplied_ids).log_field_accessis RPC-reachable and creates rows withsudo(), so any authenticated user could flood or forge the audit trail with entries pointing at arbitrary model/record/field strings. It now validates that the model and field exist, the record exists, and the caller can read it (check_access("read")). Rate limiting and a retention cron are deferred as follow-ups.size; a truncated ciphertext is undecryptable, so the plaintext would be destroyed on the very first write with no error) and translated fields (per-language jsonb storage fragments the ciphertext).search_by_blind_index/search_by_partialwere public RPC methods — a plaintext-confirmation / last-4-bucketing oracle. Renamed_-private; consumer models wrap them with their own access policy._encrypt_valueno longer swallowsAccessError(missing key permission) into a generic error; genuine crypto failures now raiseUserErrorinstead of a bareValueError.sudo()(it's a policy table) so portal/public reads of a future host model can't crash on the config ACL; the now-unneededbase.group_userread ACL on the config model is dropped (internal users can no longer enumerate the PII field map)._encryption_enabledfield — nothing read it, and it would have materialized an unexplained column on every adopting model.The same review confirmed the earlier fixes and the cross-module analysis (module is additive; installing it cannot break other modules), and produced ~19 design-level findings (ciphertext framing/key rotation,
copy()double-encryption, non-read()access paths seeing ciphertext, widget masking being cosmetic, blind-index linkage properties, config-toggle data orphaning, per-read overhead). Those are deliberately deferred: the mixin has no adopter yet, and they gate "first model adopts the mixin" (PR3 territory) rather than this migration. Tracked in #451 (items 1–5 there must land before the first model adopts the mixin).Verification
./spp t spp_pii_encryption→ all tests pass (25 tests: original 7 + 13 added in review round 1 + 5 new for the fixes), module installsrpcservice registered inweb(widget fix necessary);useris the exported object from@web/core/userspp_pii_encryption, its models, or themasked_charwidget — purely additive, mixin is opt-inNotes
write()path is covered via a config-driven test on the mixin's reflecteddisplay_namefield.string=params; oneexcept Exception: passguarding request-context extraction inaudit_log.py) were carried over unchanged from the source module.spp_change_request_v2/static/src/components/review_panel/review_panel.js:27andspp_dci_compliance/static/src/components/security_warning/security_warning.js:19still use the removeduseService("rpc")— same latent crash, worth its own issue.