diff --git a/spp_key_management/README.rst b/spp_key_management/README.rst
index eb9a3a265..a31bb296f 100644
--- a/spp_key_management/README.rst
+++ b/spp_key_management/README.rst
@@ -165,6 +165,20 @@ External Python dependencies: ``cryptography``
Changelog
=========
+19.0.2.0.1
+~~~~~~~~~~
+
+- fix(security): stop the Key Management Admin group from granting full
+ system administration. ``implied_ids`` grants the implied groups to
+ members, so the previous ``base.group_system`` link escalated any
+ holder of the key management role to a Settings/System administrator.
+ The admin group now implies Key Operator instead, key admins keep
+ field access to the KMS-wrapped key material the cloud providers read
+ in user context, and a migration strips the unsafe link from existing
+ databases. The Key Management menu moved from Settings to a top-level
+ menu: the Settings root is only visible to ERP managers, so key admins
+ who are no longer system administrators could not reach it there.
+
19.0.2.0.0
~~~~~~~~~~
diff --git a/spp_key_management/__manifest__.py b/spp_key_management/__manifest__.py
index 12b625afc..618f7e364 100644
--- a/spp_key_management/__manifest__.py
+++ b/spp_key_management/__manifest__.py
@@ -4,7 +4,7 @@
"name": "OpenSPP Key Management",
"summary": "Centralized cryptographic key management with pluggable providers",
"category": "OpenSPP/Identity",
- "version": "19.0.2.0.0",
+ "version": "19.0.2.0.1",
"sequence": 1,
"author": "OpenSPP.org",
"website": "https://github.com/OpenSPP/OpenSPP2",
diff --git a/spp_key_management/migrations/19.0.2.0.1/post-migration.py b/spp_key_management/migrations/19.0.2.0.1/post-migration.py
new file mode 100644
index 000000000..92b2bbcd5
--- /dev/null
+++ b/spp_key_management/migrations/19.0.2.0.1/post-migration.py
@@ -0,0 +1,45 @@
+# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
+"""Remove the base.group_system implication from the Key Management Admin group.
+
+Earlier versions shipped ``spp_key_management.group_key_admin`` with
+``implied_ids`` linking ``base.group_system``. ``implied_ids`` GRANTS the
+implied groups to members, so any user given the key management role
+silently became a full Settings/System administrator. Removing the link
+from the XML only stops adding it on fresh installs - it never removes an
+existing relation - so strip it here. Odoo computes effective membership
+as a transitive closure of ``implied_ids``, so removing the link
+immediately revokes the escalated privileges from affected users. (The
+replacement implication - admin implies operator - is applied by the
+regular data load; the groups file is not noupdate.)
+"""
+
+import logging
+
+from odoo import SUPERUSER_ID, Command, api
+
+_logger = logging.getLogger(__name__)
+
+
+def migrate(cr, version):
+ if not version:
+ return
+
+ env = api.Environment(cr, SUPERUSER_ID, {})
+ group = env.ref("spp_key_management.group_key_admin", raise_if_not_found=False)
+ system = env.ref("base.group_system", raise_if_not_found=False)
+ if not group or not system or system not in group.implied_ids:
+ return
+
+ # all_user_ids also counts members of any group implying key admin, so
+ # the log reflects everyone who actually held the escalation.
+ affected = len(group.all_user_ids)
+ group.write({"implied_ids": [Command.unlink(system.id)]})
+ _logger.warning(
+ "Removed the base.group_system implication from the Key Management "
+ "Admin group; %s user(s) held the group and lose the transitively "
+ "granted system administration rights. Audit changes made by these "
+ "users while escalated, grant base.group_system explicitly where it "
+ "is genuinely intended, and consider rotating data keys if key "
+ "custody policy separates platform admins from key admins.",
+ affected,
+ )
diff --git a/spp_key_management/models/encryption_key.py b/spp_key_management/models/encryption_key.py
index 27ee771c0..f80f5a6ea 100644
--- a/spp_key_management/models/encryption_key.py
+++ b/spp_key_management/models/encryption_key.py
@@ -43,7 +43,11 @@ class EncryptionKey(models.Model):
)
encrypted_key = fields.Char(
required=True,
- groups="base.group_system",
+ # Key admins need field access because the AWS KMS, Azure Key Vault
+ # and GCP KMS providers read the cached wrapped key in the calling
+ # user's context. The value is ciphertext (wrapped by the KMS);
+ # plaintext key material is never stored here.
+ groups="base.group_system,spp_key_management.group_key_admin",
help="Base64-encoded encrypted key material",
)
algorithm = fields.Char(
diff --git a/spp_key_management/readme/HISTORY.md b/spp_key_management/readme/HISTORY.md
index 4aaf9afef..cbb5293f9 100644
--- a/spp_key_management/readme/HISTORY.md
+++ b/spp_key_management/readme/HISTORY.md
@@ -1,3 +1,16 @@
+### 19.0.2.0.1
+
+- fix(security): stop the Key Management Admin group from granting full
+ system administration. `implied_ids` grants the implied groups to members,
+ so the previous `base.group_system` link escalated any holder of the key
+ management role to a Settings/System administrator. The admin group now
+ implies Key Operator instead, key admins keep field access to the
+ KMS-wrapped key material the cloud providers read in user context, and a
+ migration strips the unsafe link from existing databases. The Key
+ Management menu moved from Settings to a top-level menu: the Settings
+ root is only visible to ERP managers, so key admins who are no longer
+ system administrators could not reach it there.
+
### 19.0.2.0.0
- Initial migration to OpenSPP2
diff --git a/spp_key_management/security/security_groups.xml b/spp_key_management/security/security_groups.xml
index f5123ced8..372efc0e3 100644
--- a/spp_key_management/security/security_groups.xml
+++ b/spp_key_management/security/security_groups.xml
@@ -15,17 +15,9 @@
20
-
-
- Key Management Admin
-
- Full access to key management. Can configure providers, rotate keys, and view all key operations.
-
-
-
-
+
Key Operator
@@ -34,4 +26,28 @@
>Can use encryption keys for operations but cannot manage providers or rotate keys.
+
+
+
+ Key Management Admin
+
+ Full access to key management. Can configure providers, rotate keys, and view all key operations.
+
+
diff --git a/spp_key_management/static/description/index.html b/spp_key_management/static/description/index.html
index 10418817f..a16177040 100644
--- a/spp_key_management/static/description/index.html
+++ b/spp_key_management/static/description/index.html
@@ -548,6 +548,21 @@
+
19.0.2.0.1
+
+- fix(security): stop the Key Management Admin group from granting full
+system administration. implied_ids grants the implied groups to
+members, so the previous base.group_system link escalated any
+holder of the key management role to a Settings/System administrator.
+The admin group now implies Key Operator instead, key admins keep
+field access to the KMS-wrapped key material the cloud providers read
+in user context, and a migration strips the unsafe link from existing
+databases. The Key Management menu moved from Settings to a top-level
+menu: the Settings root is only visible to ERP managers, so key admins
+who are no longer system administrators could not reach it there.
+
+
+
19.0.2.0.0
- Initial migration to OpenSPP2
diff --git a/spp_key_management/tests/__init__.py b/spp_key_management/tests/__init__.py
index 9c906e8c9..bb85ec63b 100644
--- a/spp_key_management/tests/__init__.py
+++ b/spp_key_management/tests/__init__.py
@@ -3,3 +3,4 @@
from . import test_key_manager
from . import test_asymmetric_key
from . import test_security
+from . import test_key_admin_group
diff --git a/spp_key_management/tests/test_key_admin_group.py b/spp_key_management/tests/test_key_admin_group.py
new file mode 100644
index 000000000..82ba4d936
--- /dev/null
+++ b/spp_key_management/tests/test_key_admin_group.py
@@ -0,0 +1,162 @@
+# Part of OpenSPP. See LICENSE file for full copyright and licensing details.
+"""Tests for the Key Management security groups."""
+
+import importlib.util
+from pathlib import Path
+
+from odoo import Command
+from odoo.exceptions import AccessError
+from odoo.tests import TransactionCase, tagged
+
+
+@tagged("post_install", "-at_install")
+class TestKeyAdminGroup(TransactionCase):
+ """Key Management Admin gates control of the platform's encryption
+ keys (providers, rotation, key records).
+
+ The group must NEVER imply base.group_system: implied_ids grants the
+ implied groups to members, so such a link would turn the key
+ management role into full Settings/System administration.
+ """
+
+ def _create_user(self, login, groups):
+ return self.env["res.users"].create(
+ {
+ "name": login,
+ "login": login,
+ "group_ids": [Command.link(g.id) for g in groups],
+ }
+ )
+
+ def test_key_admin_group_exists(self):
+ group = self.env.ref("spp_key_management.group_key_admin", raise_if_not_found=False)
+ self.assertTrue(group, "spp_key_management.group_key_admin must exist")
+ self.assertEqual(group.name, "Key Management Admin")
+
+ def test_key_admin_does_not_grant_system(self):
+ """Granting Key Management Admin must not escalate to system admin."""
+ group = self.env.ref("spp_key_management.group_key_admin")
+ system = self.env.ref("base.group_system")
+ self.assertNotIn(system, group.implied_ids)
+ # Guard the transitive closure too, so the escalation cannot come
+ # back through an intermediate group.
+ self.assertNotIn(system, group.all_implied_ids)
+
+ user = self._create_user(
+ "key_admin_only",
+ [self.env.ref("base.group_user"), group],
+ )
+ self.assertTrue(user.has_group("spp_key_management.group_key_admin"))
+ self.assertFalse(
+ user.has_group("base.group_system"),
+ "Key Management Admin membership must not confer system administration",
+ )
+
+ def test_key_admin_implies_operator(self):
+ """Admin is a superset of operator (and stays an internal user)."""
+ group = self.env.ref("spp_key_management.group_key_admin")
+ operator = self.env.ref("spp_key_management.group_key_operator_officer")
+ internal = self.env.ref("base.group_user")
+ self.assertIn(operator, group.all_implied_ids)
+ self.assertIn(internal, group.all_implied_ids)
+
+ def test_key_admin_reads_encrypted_key(self):
+ """Key admins keep field access to the KMS-wrapped key material
+ (the AWS/Azure providers read it in the calling user's context);
+ operators do not."""
+ key = self.env["spp.encryption.key"].create(
+ {
+ "key_id": "test-field-gate",
+ "encrypted_key": "d3JhcHBlZA==",
+ }
+ )
+ admin_user = self._create_user(
+ "key_admin_field",
+ [
+ self.env.ref("base.group_user"),
+ self.env.ref("spp_key_management.group_key_admin"),
+ ],
+ )
+ operator_user = self._create_user(
+ "key_operator_field",
+ [
+ self.env.ref("base.group_user"),
+ self.env.ref("spp_key_management.group_key_operator_officer"),
+ ],
+ )
+ self.assertEqual(key.with_user(admin_user).encrypted_key, "d3JhcHBlZA==")
+ with self.assertRaises(AccessError):
+ key.with_user(operator_user).encrypted_key # noqa: B018
+
+ def test_system_admin_retains_key_access(self):
+ """System administrators keep key access without holding the key
+ admin group: the field gate and menu root both list
+ base.group_system, and the ACL grants system its own rows. This
+ pins the other half of the design - removing the escalation must
+ not cost real system admins anything."""
+ key = self.env["spp.encryption.key"].create(
+ {
+ "key_id": "test-system-access",
+ "encrypted_key": "d3JhcHBlZA==",
+ }
+ )
+ system_user = self._create_user(
+ "key_system_admin",
+ [self.env.ref("base.group_system")],
+ )
+ # Access is retained via base.group_system, NOT via the key group.
+ self.assertFalse(system_user.has_group("spp_key_management.group_key_admin"))
+ self.assertEqual(key.with_user(system_user).encrypted_key, "d3JhcHBlZA==")
+ root = self.env.ref("spp_key_management.menu_key_management_root")
+ menus = self.env["ir.ui.menu"].with_user(system_user).load_menus(False)
+ self.assertIn(root.id, menus)
+
+ def test_key_admin_sees_key_management_menu(self):
+ """A user holding only the key admin role must be able to reach the
+ Key Management menu. Odoo drops menus whose ancestors are invisible,
+ so parenting under Settings (base.group_erp_manager) would hide the
+ whole subtree from dedicated key-custody users."""
+ user = self._create_user(
+ "key_admin_menu",
+ [
+ self.env.ref("base.group_user"),
+ self.env.ref("spp_key_management.group_key_admin"),
+ ],
+ )
+ root = self.env.ref("spp_key_management.menu_key_management_root")
+ # load_menus is what the web client uses; unlike _visible_menu_ids
+ # it also prunes menus whose ancestors are invisible ("not related
+ # to an app"), which is exactly the failure mode being pinned.
+ menus = self.env["ir.ui.menu"].with_user(user).load_menus(False)
+ self.assertIn(
+ root.id,
+ menus,
+ "Key Management menu must be reachable for key admins",
+ )
+
+ def test_migration_removes_escalation(self):
+ """The 19.0.2.0.1 migration strips base.group_system from the
+ group on databases installed before the fix (removing the XML
+ line alone never unlinks an existing relation)."""
+ migration_path = Path(__file__).parent.parent / "migrations" / "19.0.2.0.1" / "post-migration.py"
+ spec = importlib.util.spec_from_file_location("spp_key_management_migration_19_0_2_0_1", migration_path)
+ migration = importlib.util.module_from_spec(spec)
+ spec.loader.exec_module(migration)
+
+ group = self.env.ref("spp_key_management.group_key_admin")
+ system = self.env.ref("base.group_system")
+ # Recreate the released (vulnerable) state.
+ group.write({"implied_ids": [Command.link(system.id)]})
+ self.assertIn(system, group.implied_ids)
+
+ with self.assertLogs("spp_key_management_migration_19_0_2_0_1", level="WARNING") as capture:
+ migration.migrate(self.env.cr, "19.0.2.0.0")
+ self.assertIn("Removed the base.group_system implication", capture.output[0])
+
+ group.invalidate_recordset()
+ self.assertNotIn(system, group.implied_ids)
+ self.assertNotIn(system, group.all_implied_ids)
+
+ # Idempotent: a second run finds the link absent and stays silent.
+ with self.assertNoLogs("spp_key_management_migration_19_0_2_0_1", level="WARNING"):
+ migration.migrate(self.env.cr, "19.0.2.0.0")
diff --git a/spp_key_management/views/menu.xml b/spp_key_management/views/menu.xml
index b9d7d56a2..16bb508c7 100644
--- a/spp_key_management/views/menu.xml
+++ b/spp_key_management/views/menu.xml
@@ -1,10 +1,14 @@
-
+