feat(spp_pii_encryption): re-add migration wizard; mask registry ID numbers - #452
Merged
Merged
Conversation
Member
Author
|
Reviewer note: read this together with #451 (deferred findings from the #232 adversarial review).
|
…y-run/migrate)
Ported from openspp-modules, unblocked by spp_data_classification
(is_pii): the scan is driven by the classification registry.
Deliberate changes from the source:
- No in-app rollback and no plaintext backup table. The rollback relied
on a skip_encryption context no mixin implements (it re-encrypted on
write), and a persistent plaintext copy of the values being encrypted
contradicts ADR-012 threat model ("backup exposure"). The Migrate
confirm dialog now demands a database snapshot instead.
- Migration loops batches until each field is exhausted, with failed
records excluded from the search domain so one broken record cannot
loop forever; previously only the first batch was processed while the
summary claimed completion.
- Scan reads the classification's stored model_name and tolerates
AccessError per model (logged as skipped) — the encryption admin is
deliberately not a system admin and cannot read ir.model records.
- batch_size validated positive; user-facing strings translated.
Tests register a concrete mixin consumer via a vendored Odoo-19
adaptation of odoo-test-helper's FakeModelLoader (the released helper
targets pre-19 registry internals); legacy plaintext rows are fabricated
with direct SQL since ORM creates auto-encrypt.
Module version 19.0.2.0.0 (clears the openspp-modules 19.0.1.0.0
baseline that included the wizard).
…d reveal New applier module wiring the masked_char widget (spp_pii_encryption) into the individual and group Identity tabs: spp.registry.id values render as ••••-••••-1234, and revealing them requires spp_data_classification.group_pii_full_access_admin — the PII access group the RESTRICTED classification level points at — with every reveal written to the PII access audit log. Display masking only: encrypting the stored values (mixin on spp.registry.id) is a separate change with search/dedup impact. spp_registry itself stays free of upward dependencies.
gonzalesedwin1123
force-pushed
the
migrate-spp-pii-encryption-pr3
branch
from
August 25, 2026 06:00
3f39bf6 to
33b4e3b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #452 +/- ##
==========================================
- Coverage 76.72% 76.66% -0.06%
==========================================
Files 634 631 -3
Lines 42625 42514 -111
==========================================
- Hits 32703 32593 -110
+ Misses 9922 9921 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…tion spp_pii_encryption files applied verbatim from the CI pre-commit run (32815132448). spp_registry_encryption files bootstrapped via the pinned oca-gen hook in a pre-commit-managed env (CI cannot print a diff for files that do not exist yet); if CI renders them differently, its next printed diff is authoritative.
Per the widget-honesty findings in the spp_pii_encryption hardening tracker (#451 items 6-7): the widget masks readonly display and audits reveals through its control, but entering the editable cell shows the value and the plaintext reaches the browser via the normal record read either way. The description now says exactly that — the access boundary remains record ACLs; the mask buys shoulder-surfing protection and an audit trail for deliberate reveals. Mask keeps the last-4 pattern the classification registry itself seeds for national IDs; a platform-wide default-mask change belongs to #451 item 7.
Applied verbatim from CI pre-commit run 32817863234.
Two leaks found while browser-testing the first real wiring (spp_registry_encryption, individual form Identity tab): - The reveal toggle click bubbled to the list cell, so the "gate" button itself opened the row editor and exposed the plaintext input with no permission check and no audit entry. The click now stays on the toggle (t-on-click.stop); entering edit mode remains possible by clicking the cell directly, which is the documented de-emphasis behavior. - The list renderer copies every char cell's formatted (raw) value into the cell's data-tooltip for truncated columns, so hovering a masked cell showed the full plaintext. masked_char columns now get no cell tooltip (ListRenderer.getCellTitle patch). Verified in a live browser (headless Chrome against the dev stack): masked render in the editable list, reveal refused without the PII group (toast, no audit row), reveal + spp.pii.audit.log row with it, edit-through-widget persists and re-masks on save; no console errors.
Applied verbatim from CI pre-commit run 32822423156.
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
PR3 of the PII sequence — re-adds the encryption migration wizard to
spp_pii_encryptionand wires themasked_charwidget into a real view via a new applier module. Completes the ADR-011/ADR-012 foundation chain:spp_pii_encryptioncorespp_data_classificationregistry1.
spp_pii_encryption→ 19.0.2.0.0: migration wizard (scan / dry-run / migrate)Ported from openspp-modules with deliberate changes:
skip_encryptioncontext no mixin implements (writing plaintext back just re-encrypts it), and the backup model persisted the exact plaintext values being encrypted, unencrypted, indefinitely — directly against ADR-012's own threat model ("Backup exposure → Encrypted backups"). The Migrate button now demands a database snapshot instead.batch_sizerecords while reporting "Migration complete"; the wizard now loops until each field is exhausted, excluding failed records from the domain so one broken record can't loop forever, flushing/invalidating per batch.ir.modelrecords are readable only by the Access Rights group, so the scan reads the classification's storedmodel_nameand logs-and-skips models the operator can'tsearch_count(the encryption admin is deliberately not a system admin — see feat(spp_pii_encryption): port PII encryption core from openspp-modules #232).batch_sizevalidated positive, user-facing strings translated, and themodel_name/model_id"Model" label clash onspp.field.encryption.configfixed (same fix as feat(spp_data_classification): port classification registry from openspp-modules #233's registry models).Version goes to 19.0.2.0.0 to clear the openspp-modules baseline (19.0.1.0.0, which shipped the wizard); HISTORY fragment included.
2. New module:
spp_registry_encryption(19.0.1.0.0)First consumer of the PR2 groups and the PR1 widget: registrant ID numbers (
spp.registry.id.value) render masked (••••-••••-1234) in both Identity tabs ofspp_registry.view_individuals_form, with a reveal control gated byspp_data_classification.group_pii_full_access_adminand each reveal audited inspp.pii.audit.log.Framing (per the widget-honesty findings, #451 items 6–7): this is display de-emphasis, not an access control. The widget masks readonly display; entering the editable cell shows the value, and the plaintext reaches the browser via the normal record read either way — the access boundary remains the record's ACLs. What it buys is shoulder-surfing/casual-exposure protection plus an audit trail for deliberate reveals. The module description states this explicitly. The mask keeps the last-4 pattern the classification registry itself seeds for national IDs; changing the platform default mask is #451 item 7. Server-side field-level enforcement is #451 item 6 — when it lands, this module is where the registry adopts it.
Display masking only — actually encrypting
spp.registry.id.value(mixin + blind index) has search/dedup impact and is a separate change.spp_registryitself stays free of upward dependencies.Test infrastructure note
No concrete model inherits
spp.encrypted.field.mixinyet, so the wizard tests register a throwaway consumer via a vendored, Odoo-19-adapted FakeModelLoader (tests/fake_model_loader.py) — the releasedodoo-test-helper(2.1.3) targets pre-19 registry internals (MetaModel.module_to_models,Registry.setup_models) and cannot import on 19. Scoped to adding brand-new models; to be replaced when the helper supports Odoo 19. Legacy plaintext rows are fabricated with parameterized SQL, since ORM creates auto-encrypt.Verification
./spp t spp_pii_encryption→ 47/47 (7 original mixin tests + wizard suite: end-to-end encrypt of legacy plaintext, blind-index creation, read() roundtrip, batch looping, per-record failure isolation, unreadable-model skip, dry-run immutability)./spp t spp_registry_encryption→ 2/2 (get_view validates the inherited arch and asserts the widget on both tabs — this also proves Odoo 19 view validation accepts the widget's XML attributes)./spp lintgreen; module load logs clean (label-clash warning gone)Notes
applyMask, tour test for reveal→audit, job-worker-based async migration for very large tables.