From 889c78ecb02fee854540a2f457f55ad14b3ba9ce Mon Sep 17 00:00:00 2001
From: Tony Meyer <tony.meyer@canonical.com>
Date: Fri, 24 Nov 2023 10:07:12 +1300
Subject: [PATCH] fix: non-leaders can only view app secrets (#1076)

Non-leaders can only view secrets.
---
 ops/testing.py       | 14 ++++++++++---
 test/test_testing.py | 47 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/ops/testing.py b/ops/testing.py
index 607e2a501..fe0f71ffd 100644
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -2482,9 +2482,17 @@ 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]:
-            raise model.SecretNotFoundError(
-                f'You must own secret {secret.id!r} to perform this operation')
+        # 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
+        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,
diff --git a/test/test_testing.py b/test/test_testing.py
index fdf3000de..3ebbd4c91 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,52 @@ def test_trigger_secret_expiration(self):
         with self.assertRaises(RuntimeError):
             harness.trigger_secret_removal('nosecret', 1)
 
+    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)
+        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()
+
+    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
+        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()
+
+    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
+        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):