security(key_management): stop Key Management Admin group from granting system admin

[gonzalesedwin1123]

Do not merge yet — held for human review.

Summary

spp_key_management.group_key_admin ("Key Management Admin") was defined with implied_ids linking base.group_system. In Odoo, implied_ids grants the implied groups to members — it is not a membership precondition — so assigning the key management role silently made that user a full Settings/System administrator. The link shipped with the initial platform export and is present in both released tags (v19.0.2.0.0, 2026.07). This is the same bug shape fixed for the DCI group in #327; a repo-wide sweep (multi-line-aware) confirms these were the only two instances.

Unlike the DCI case, the escalation was load-bearing: spp.encryption.key.encrypted_key (the KMS-wrapped key material) is field-gated, and the AWS KMS / Azure Key Vault providers read and write it in the calling user's context (the database provider sudo's; those two don't) — so key admins could only use those providers because they were escalated to system.

Fix

• Remove the escalation: group_key_admin no longer implies base.group_system. It implies group_key_operator_officer instead (admin ⊇ operator; operator implies base.group_user, so a key admin stays an internal user). The groups file is reordered so the operator group is defined before the admin group that now references it.
• Deliberately NOT linked into spp_security.group_spp_admin (divergence from security(dci): stop DCI Administrator group from granting system admin #327): system admins already hold their own explicit ACL rows on every key model and the menu lists both groups, so they lose nothing — while the conventional link would newly grant key rotation and provider configuration to every OpenSPP admin. Least privilege wins for the encryption control-plane.
• Preserve key-admin function on the cloud providers: encrypted_key's field gate now includes group_key_admin. This is a strict reduction of that role's access (from "entire system" to "key models + KMS-wrapped ciphertext"); plaintext key material is never stored in that field.
• Migration for released databases: migrations/19.0.2.0.1/post-migration.py strips the link via the ORM and logs the count of affected users. Removing the XML line alone never unlinks an existing relation (the file is not noupdate, but data loads only add Command.link entries); Odoo 19 computes membership as a transitive closure, so the migration revokes the escalation immediately. The new admin→operator implication is applied by the regular data load.
• Menu reachability (adversarial-review finding): the Key Management root menu moved from Settings to top level. The Settings root is gated to base.group_erp_manager and Odoo prunes menus whose ancestors are invisible, so a dedicated key admin (no longer a system admin) had no way to reach Key Management in the UI. Pinned by a load_menus-based test; disclosed in HISTORY.
• Version bump 19.0.2.0.0 → 19.0.2.0.1 + HISTORY.md fragment.

Tests (TDD — red commit first; the module had no group tests at all)

New tests/test_key_admin_group.py:

• test_key_admin_does_not_grant_system: link absent from implied_ids and all_implied_ids (blocks reintroduction through an intermediate group); behavioral: a user granted the group does not has_group('base.group_system').
• test_key_admin_implies_operator: operator and internal-user membership via the closure.
• test_key_admin_reads_encrypted_key: key admin reads the wrapped key field (guards the AWS/Azure provider paths); operator is denied (pins the field gate).
• test_migration_removes_escalation: recreates the released state, runs migrate() under assertLogs, asserts the link and its closure effects are gone and the operator-facing warning is emitted.
• Green: spp_key_management 75/75, dependents spp_encryption 26/26 and spp_claim_169 26/26. Lint (ruff/prettier/pylint-odoo) clean; security audit 0 errors / 0 warnings.

Operational note for deployments

On v19.0.2.0.0 / 2026.07 installs, anyone granted Key Management Admin has been a de-facto system admin since the grant. The migration revokes the privilege on upgrade, but operators should audit actions taken by those users while escalated. Additionally — unlike the DCI case — such users had system-level access to KMS-wrapped key material and provider configuration, so if key-custody policy assumes separation between platform admins and key admins, consider rotating data keys after the upgrade.

Known gap (pre-existing, not addressed here)

Key operators cannot use the AWS KMS / Azure Key Vault provider paths (model read ACL but no field access to encrypted_key, and those providers don't sudo like the database provider does). This predates this PR — those paths only ever worked for system-level users — and will be filed separately rather than mixing a functional repair into this security fix.

Reviewed

Adversarial staff review: the group rework, migration, and tests were confirmed sound against Odoo 19 source (sequential ref() resolution makes the operator-first reorder necessary and sufficient; MigrationManager bounds run the script exactly on the 2.0.0 → 2.0.1 upgrade; field-level groups raise AccessError on non-member attribute reads even with a warm cache, so the operator-denied test cannot false-pass; the operator implication grants nothing beyond internal-user status — its other ACL rows were already internal-user-readable metadata). On the critical field-gate question: for the database provider the stored value is AES-GCM ciphertext under a KEK from env/config unreachable via the ORM, and key admins already receive plaintext DEKs through the sanctioned KeyManager API (_check_key_access allows them all operations), so field access to ciphertext is strictly weaker than what the role legitimately has — not an escalation. The review's one confirmed problem — key admins losing all UI access to Key Management because the menu lived under the erp-manager-gated Settings root — is fixed in this PR (menu re-parented to top level, test-pinned, disclosed in HISTORY); its minor notes (GCP KMS also reads the field in user context; migration log counting; migration idempotence coverage) are addressed. The pre-existing asymmetric-key field-gate inconsistency and operator-on-KMS gap remain out of scope as documented.

Merge order & release-note note

Per the cross-PR interaction analysis (2026-07-24, internal/plans/security-prs-interaction-analysis.md): no hard ordering — every combination with the other open security PRs commutes (verified: key-management workflows don't regress; key admins have their own ACL rows and _check_key_access checks the group independently; the two system-gated points are fixed in this PR). Soft preference: land this early (or in the same release as #336/#265) — until the implication is stripped, key admins still transit base.group_system and pass those PRs' new gates.

Release notes must state: Key Management Admin no longer grants system admin; in combination with #265, OAuth signing-key rotation now requires explicit base.group_system (the two key domains are deliberately separate — OAuth JWT keys live in ir.config_parameter via spp_oauth, not in spp.encryption.key). The migration logs a warning, but it belongs in the notes.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.67%. Comparing base (ec4493d) to head (180c7fc).

Additional details and impacted files

@@                     Coverage Diff                     @@
##           19.0-staging-sec-batch1     #329      +/-   ##
===========================================================
- Coverage                    75.84%   70.67%   -5.17%     
===========================================================
  Files                          188      219      +31     
  Lines                        12492    17348    +4856     
===========================================================
+ Hits                          9474    12261    +2787     
- Misses                        3018     5087    +2069     

Flag	Coverage Δ
spp_attachment_av_scan	83.98% <ø> (?)
spp_base_common	91.07% <ø> (ø)
spp_claim_169	58.18% <ø> (?)
spp_dci_compliance	?
spp_dci_demo	94.28% <ø> (ø)
spp_encryption	55.79% <ø> (?)
spp_key_management	38.52% <ø> (?)
spp_mis_demo_v2	70.99% <ø> (?)
spp_programs	65.27% <ø> (ø)
spp_registry	86.94% <ø> (ø)
spp_security	69.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
spp_key_management/models/encryption_key.py	91.66% <ø> (ø)

... and 55 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[gonzalesedwin1123]

Staff-review follow-ups filed as separate issues (all pre-existing, deliberately out of this PR's scope):

• security(spp_key_management): KMS providers read spp.encryption.key in user context; operators broken, field gate over-broad [Severity: Low] #330 — KMS providers read spp.encryption.key in user context (operators broken on KMS paths; field gate over-broad)
• security(spp_key_management): AWS KMS decrypt omits explicit KeyId, allowing key substitution by a key custodian [Severity: Low] #331 — AWS KMS decrypt omits explicit KeyId (key substitution by a custodian)
• security(spp_encryption): group_crypto_manager implies group_spp_admin but has no key-model ACLs [Severity: Low] #332 — spp_encryption.group_crypto_manager implies group_spp_admin but has no key-model ACLs
• security(spp_key_management): encrypted_private_key lacks a field-level groups gate (defense-in-depth) [Severity: Low] #333 — encrypted_private_key lacks a field-level gate (defense-in-depth)
• security(ci): guard against res.groups implying base.group_system outside spp_security (privilege-escalation class) #334 — CI guard against implied_ids → base.group_system (prevents a third instance of this escalation class)

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.