From 793bd999dcf7a9ad2e2b1adeb480e8ae405afd6c Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 23 Nov 2023 18:41:02 +1300 Subject: [PATCH 1/3] Non-leaders can only view secrets. --- ops/testing.py | 7 ++++++- test/test_testing.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/ops/testing.py b/ops/testing.py index 607e2a501..e3a8a2750 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -2482,7 +2482,12 @@ def _relation_id_to(self, remote_app: str) -> Optional[int]: return None def _ensure_secret_owner(self, secret: _Secret): - if secret.owner_name not in [self.app_name, self.unit_name]: + # For unit secrets, the owner unit has manage permissions. For app + # secrets, the leader has manage permissions and other units only have + # view permissions. + # https://discourse.charmhub.io/t/secret-access-permissions/12627 + if (secret.owner_name != self.unit_name + and (secret.owner_name != self.app_name or not self.is_leader())): raise model.SecretNotFoundError( f'You must own secret {secret.id!r} to perform this operation') diff --git a/test/test_testing.py b/test/test_testing.py index fdf3000de..3425ef962 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -4819,6 +4819,7 @@ def test_get_secret_grants(self): harness.add_relation_unit(relation_id, 'webapp/0') assert harness is not None + harness.set_leader(True) secret = harness.model.app.add_secret({'foo': 'x'}) assert secret.id is not None self.assertEqual(harness.get_secret_grants(secret.id, relation_id), set()) @@ -4914,6 +4915,39 @@ def test_trigger_secret_expiration(self): with self.assertRaises(RuntimeError): harness.trigger_secret_removal('nosecret', 1) + def test_secret_permissions(self): + harness = ops.testing.Harness(ops.CharmBase, meta='name: database') + self.addCleanup(harness.cleanup) + harness.begin() + # The charm can always manage a local unit secret. + secret_id = harness.charm.unit.add_secret({"password": "1234"}).id + secret = harness.charm.model.get_secret(id=secret_id) + self.assertEqual(secret.get_content(), {"password": "1234"}) + info = secret.get_info() + self.assertEqual(info.id, secret_id) + secret.set_content({"password": "5678"}) + secret.remove_all_revisions() + # The leader can manage an application secret. + harness.set_leader(True) + secret_id = harness.charm.app.add_secret({"password": "1234"}).id + secret = harness.charm.model.get_secret(id=secret_id) + self.assertEqual(secret.get_content(), {"password": "1234"}) + info = secret.get_info() + self.assertEqual(info.id, secret_id) + secret.set_content({"password": "5678"}) + secret.remove_all_revisions() + # Non-leaders can only view an application secret. + harness.set_leader(False) + secret_id = harness.charm.app.add_secret({"password": "1234"}).id + secret = harness.charm.model.get_secret(id=secret_id) + self.assertEqual(secret.get_content(), {"password": "1234"}) + with self.assertRaises(ops.model.SecretNotFoundError): + secret.get_info() + with self.assertRaises(ops.model.SecretNotFoundError): + secret.set_content({"password": "5678"}) + with self.assertRaises(ops.model.SecretNotFoundError): + secret.remove_all_revisions() + class EventRecorder(ops.CharmBase): def __init__(self, framework: ops.Framework): From 96bdc0639e235eaa20ea92e1bdb0fbd4489d5475 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 23 Nov 2023 23:15:31 +1300 Subject: [PATCH 2/3] Split test as per review comment. --- test/test_testing.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/test/test_testing.py b/test/test_testing.py index 3425ef962..3ebbd4c91 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -4915,10 +4915,11 @@ def test_trigger_secret_expiration(self): with self.assertRaises(RuntimeError): harness.trigger_secret_removal('nosecret', 1) - def test_secret_permissions(self): + def test_secret_permissions_unit(self): harness = ops.testing.Harness(ops.CharmBase, meta='name: database') self.addCleanup(harness.cleanup) harness.begin() + # The charm can always manage a local unit secret. secret_id = harness.charm.unit.add_secret({"password": "1234"}).id secret = harness.charm.model.get_secret(id=secret_id) @@ -4927,6 +4928,12 @@ def test_secret_permissions(self): self.assertEqual(info.id, secret_id) secret.set_content({"password": "5678"}) secret.remove_all_revisions() + + def test_secret_permissions_leader(self): + harness = ops.testing.Harness(ops.CharmBase, meta='name: database') + self.addCleanup(harness.cleanup) + harness.begin() + # The leader can manage an application secret. harness.set_leader(True) secret_id = harness.charm.app.add_secret({"password": "1234"}).id @@ -4936,6 +4943,12 @@ def test_secret_permissions(self): self.assertEqual(info.id, secret_id) secret.set_content({"password": "5678"}) secret.remove_all_revisions() + + def test_secret_permissions_nonleader(self): + harness = ops.testing.Harness(ops.CharmBase, meta='name: database') + self.addCleanup(harness.cleanup) + harness.begin() + # Non-leaders can only view an application secret. harness.set_leader(False) secret_id = harness.charm.app.add_secret({"password": "1234"}).id From 9206a77173ea1722355399c3bb93b2dbfe12e918 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 23 Nov 2023 23:22:20 +1300 Subject: [PATCH 3/3] Rework logic to focus on the positive case, as per review comment. --- ops/testing.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ops/testing.py b/ops/testing.py index e3a8a2750..fe0f71ffd 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -2486,10 +2486,13 @@ def _ensure_secret_owner(self, secret: _Secret): # secrets, the leader has manage permissions and other units only have # view permissions. # https://discourse.charmhub.io/t/secret-access-permissions/12627 - if (secret.owner_name != self.unit_name - and (secret.owner_name != self.app_name or not self.is_leader())): - raise model.SecretNotFoundError( - f'You must own secret {secret.id!r} to perform this operation') + unit_secret = secret.owner_name == self.unit_name + app_secret = secret.owner_name == self.app_name + + if unit_secret or (app_secret and self.is_leader()): + return + raise model.SecretNotFoundError( + f'You must own secret {secret.id!r} to perform this operation') def secret_info_get(self, *, id: Optional[str] = None,