fix: adjust Harness secret behaviour to align with Juju

ops/charm.py

@@ -841,7 +841,11 @@ def __init__(self, handle: 'Handle', id: str, label: Optional[str]):
 
     @property
     def secret(self) -> model.Secret:
-        """The secret instance this event refers to."""
+        """The secret instance this event refers to.
+
+        Note that the secret content is not retrieved from the secret storage
+        until :meth:`Secret.get_content()` is called.
+        """
         backend = self.framework.model._backend
         return model.Secret(backend=backend, id=self._id, label=self._label)
 
@@ -901,9 +905,10 @@ class SecretRemoveEvent(SecretEvent):
     observers have updated to that new revision, this event will be fired to
     inform the secret owner that the old revision can be removed.
 
-    Typically, the charm will call
+    After any required cleanup, the charm should call
     :meth:`event.secret.remove_revision() <ops.Secret.remove_revision>` to
-    remove the now-unused revision.
+    remove the now-unused revision. If the charm does not, then the event will
+    be emitted again, when further revisions are ready for removal.
     """
 
     def __init__(self, handle: 'Handle', id: str, label: Optional[str], revision: int):

ops/framework.py

@@ -953,7 +953,8 @@ def _reemit(self, single_event_path: Optional[str] = None):
                 logger.warning(
                     'Reference to ops.Object at path %s has been garbage collected '
                     'between when the charm was initialised and when the event was emitted. '
-                    'Make sure sure you store a reference to the observer.', observer_path
+                    'Make sure sure you store a reference to the observer.',
+                    observer_path,
                 )
 
             if event.deferred:

ops/model.py

@@ -269,13 +269,19 @@ def get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) -
         owners set a label using ``add_secret``, whereas secret observers set
         a label using ``get_secret`` (see an example at :attr:`Secret.label`).
 
+        The content of the secret is retrieved, so calls to
+        :meth:`Secret.get_content` do not require querying the secret storage
+        again, unless ``refresh=True`` is used, or :meth:`Secret.set_content`
+        has been called.
+
         Args:
             id: Secret ID if fetching by ID.
             label: Secret label if fetching by label (or updating it).
 
         Raises:
-            SecretNotFoundError: If a secret with this ID or label doesn't exist,
-                or the caller does not have permission to view it.
+            SecretNotFoundError: If a secret with this ID or label doesn't exist.
+            ModelError: if the charm does not have permission to access the
+                secret.
         """
         if not (id or label):
             raise TypeError('Must provide an id or label, or both')
@@ -1365,13 +1371,22 @@ def _on_secret_changed(self, event):
     def get_content(self, *, refresh: bool = False) -> Dict[str, str]:
         """Get the secret's content.
 
+        The content of the secret is cached on the :class:`Secret` object, so
+        subsequent calls do not require querying the secret storage again,
+        unless ``refresh=True`` is used, or :meth:`set_content` is called.
+
         Returns:
             A copy of the secret's content dictionary.
 
         Args:
             refresh: If true, fetch the latest revision's content and tell
                 Juju to update to tracking that revision. The default is to
                 get the content of the currently-tracked revision.
+
+        Raises:
+            SecretNotFoundError: if the secret no longer exists.
+            ModelError: if the charm does not have permission to access the
+                secret.
         """
         if refresh or self._content is None:
             self._content = self._backend.secret_get(id=self.id, label=self.label, refresh=refresh)
@@ -1381,14 +1396,24 @@ def peek_content(self) -> Dict[str, str]:
         """Get the content of the latest revision of this secret.
 
         This returns the content of the latest revision without updating the
-        tracking.
+        tracking. The content is not cached locally, so multiple calls will
+        result in multiple queries to the secret storage.
+
+        Raises:
+            SecretNotFoundError: if the secret no longer exists.
+            ModelError: if the charm does not have permission to access the
+                secret.
         """
         return self._backend.secret_get(id=self.id, label=self.label, peek=True)
 
     def get_info(self) -> SecretInfo:
         """Get this secret's information (metadata).
 
         Only secret owners can fetch this information.
+
+        Raises:
+            SecretNotFoundError: if the secret no longer exists, or if the charm
+                does not have permission to access the secret.
         """
         return self._backend.secret_info_get(id=self.id, label=self.label)
 
@@ -1399,6 +1424,10 @@ def set_content(self, content: Dict[str, str]):
         the secret (the "observers") that a new revision is available with a
         :class:`ops.SecretChangedEvent <ops.charm.SecretChangedEvent>`.
 
+        If the charm does not have permission to update the secret, or the
+        secret no longer exists, this method will succeed, but the unit will go
+        into error state on completion of the current Juju hook.
+
         Args:
             content: A key-value mapping containing the payload of the secret,
                 for example :code:`{"password": "foo123"}`.
@@ -1407,7 +1436,8 @@ def set_content(self, content: Dict[str, str]):
         if self._id is None:
             self._id = self.get_info().id
         self._backend.secret_set(typing.cast(str, self.id), content=content)
-        self._content = None  # invalidate cache so it's refetched next get_content()
+        # We do not need to invalidate the cache here, as the content is the
+        # same until `refresh` is used, at which point the cache is invalidated.
 
     def set_info(
         self,
@@ -1422,6 +1452,10 @@ def set_info(
         This will not create a new secret revision (that applies only to
         :meth:`set_content`). Once attributes are set, they cannot be unset.
 
+        If the charm does not have permission to update the secret, or the
+        secret no longer exists, this method will succeed, but the unit will go
+        into error state on completion of the current Juju hook.
+
         Args:
             label: New label to apply.
             description: New description to apply.
@@ -1480,14 +1514,17 @@ def revoke(self, relation: 'Relation', *, unit: Optional[Unit] = None):
     def remove_revision(self, revision: int):
         """Remove the given secret revision.
 
-        This is normally called when handling
+        This is normally only called when handling
         :class:`ops.SecretRemoveEvent <ops.charm.SecretRemoveEvent>` or
         :class:`ops.SecretExpiredEvent <ops.charm.SecretExpiredEvent>`.
 
+        If the charm does not have permission to remove the secret, or it no
+        longer exists, this method will succeed, but the unit will go into error
+        state on completion of the current Juju hook.
+
         Args:
-            revision: The secret revision to remove. If being called from a
-                secret event, this should usually be set to
-                :attr:`SecretRemoveEvent.revision`.
+            revision: The secret revision to remove. This should usually be set to
+                :attr:`SecretRemoveEvent.revision` or :attr:`SecretExpiredEvent.revision`.
         """
         if self._id is None:
             self._id = self.get_info().id
@@ -1498,6 +1535,10 @@ def remove_all_revisions(self) -> None:
 
         This is called when the secret is no longer needed, for example when
         handling :class:`ops.RelationBrokenEvent <ops.charm.RelationBrokenEvent>`.
+
+        If the charm does not have permission to remove the secret, or it no
+        longer exists, this method will succeed, but the unit will go into error
+        state on completion of the current Juju hook.
         """
         if self._id is None:
             self._id = self.get_info().id

ops/testing.py

@@ -2741,29 +2741,30 @@ def _relation_id_to(self, remote_app: str) -> Optional[int]:
                 return relation_id
         return None
 
-    def _ensure_secret_owner(self, secret: _Secret):
+    def _has_secret_owner_permission(self, secret: _Secret) -> bool:
         # 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
         # For user secrets the secret owner is the model, that is,
-        # `secret.owner_name == self.model.uuid`, only model admins have
+        # when `secret.owner_name == self.model.uuid`, only model admins have
         # manage permissions: https://juju.is/docs/juju/secret.
 
         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'
-        )
+            return True
+        return False
 
     def secret_info_get(
         self, *, id: Optional[str] = None, label: Optional[str] = None
     ) -> model.SecretInfo:
         secret = self._ensure_secret_id_or_label(id, label)
-        self._ensure_secret_owner(secret)
+        if not self._has_secret_owner_permission(secret):
+            raise model.SecretNotFoundError(
+                f'You must own secret {secret.id!r} to perform this operation'
+            )
 
         rotates = None
         rotation = None
@@ -2793,7 +2794,8 @@ def secret_set(
         rotate: Optional[model.SecretRotate] = None,
     ) -> None:
         secret = self._ensure_secret(id)
-        self._ensure_secret_owner(secret)
+        if not self._has_secret_owner_permission(secret):
+            raise RuntimeError(f'You must own secret {secret.id!r} to perform this operation')
 
         if content is None:
             content = secret.revisions[-1].content
@@ -2866,7 +2868,10 @@ def _secret_add(
 
     def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None) -> None:
         secret = self._ensure_secret(id)
-        self._ensure_secret_owner(secret)
+        if not self._has_secret_owner_permission(secret):
+            raise model.SecretNotFoundError(
+                f'You must own secret {secret.id!r} to perform this operation'
+            )
 
         if relation_id not in secret.grants:
             secret.grants[relation_id] = set()
@@ -2875,7 +2880,8 @@ def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None)
 
     def secret_revoke(self, id: str, relation_id: int, *, unit: Optional[str] = None) -> None:
         secret = self._ensure_secret(id)
-        self._ensure_secret_owner(secret)
+        if not self._has_secret_owner_permission(secret):
+            raise RuntimeError(f'You must own secret {secret.id!r} to perform this operation')
 
         if relation_id not in secret.grants:
             return
@@ -2884,7 +2890,8 @@ def secret_revoke(self, id: str, relation_id: int, *, unit: Optional[str] = None
 
     def secret_remove(self, id: str, *, revision: Optional[int] = None) -> None:
         secret = self._ensure_secret(id)
-        self._ensure_secret_owner(secret)
+        if not self._has_secret_owner_permission(secret):
+            raise RuntimeError(f'You must own secret {secret.id!r} to perform this operation')
 
         if revision is not None:
             revisions = [r for r in secret.revisions if r.revision != revision]

test/test_model.py

@@ -3663,24 +3663,6 @@ def test_get_content_copies_dict(self, model: ops.Model, fake_script: FakeScript
 
         assert fake_script.calls(clear=True) == [['secret-get', 'secret:z', '--format=json']]
 
-    def test_set_content_invalidates_cache(self, model: ops.Model, fake_script: FakeScript):
-        fake_script.write('secret-get', """echo '{"foo": "bar"}'""")
-        fake_script.write('secret-set', """exit 0""")
-
-        secret = self.make_secret(model, id='z')
-        old_content = secret.get_content()
-        assert old_content == {'foo': 'bar'}
-        secret.set_content({'new': 'content'})
-        fake_script.write('secret-get', """echo '{"new": "content"}'""")
-        new_content = secret.get_content()
-        assert new_content == {'new': 'content'}
-
-        assert fake_script.calls(clear=True) == [
-            ['secret-get', 'secret:z', '--format=json'],
-            ['secret-set', 'secret:z', 'new=content'],
-            ['secret-get', 'secret:z', '--format=json'],
-        ]
-
     def test_peek_content(self, model: ops.Model, fake_script: FakeScript):
         fake_script.write('secret-get', """echo '{"foo": "peeked"}'""")
 

test/test_testing.py

@@ -5782,9 +5782,9 @@ def test_secret_permissions_nonleader(self, request: pytest.FixtureRequest):
         assert secret.get_content() == {'password': '1234'}
         with pytest.raises(ops.model.SecretNotFoundError):
             secret.get_info()
-        with pytest.raises(ops.model.SecretNotFoundError):
+        with pytest.raises(RuntimeError):
             secret.set_content({'password': '5678'})
-        with pytest.raises(ops.model.SecretNotFoundError):
+        with pytest.raises(RuntimeError):
             secret.remove_all_revisions()
 
     def test_add_user_secret(self, request: pytest.FixtureRequest):
@@ -5805,7 +5805,7 @@ def test_get_user_secret_without_grant(self, request: pytest.FixtureRequest):
         request.addfinalizer(harness.cleanup)
         harness.begin()
         secret_id = harness.add_user_secret({'password': 'foo'})
-        with pytest.raises(ops.SecretNotFoundError):
+        with pytest.raises(ops.ModelError):
             harness.model.get_secret(id=secret_id)
 
     def test_revoke_user_secret(self, request: pytest.FixtureRequest):
@@ -5817,7 +5817,7 @@ def test_revoke_user_secret(self, request: pytest.FixtureRequest):
         secret_id = harness.add_user_secret(secret_content)
         harness.grant_secret(secret_id, 'webapp')
         harness.revoke_secret(secret_id, 'webapp')
-        with pytest.raises(ops.SecretNotFoundError):
+        with pytest.raises(ops.ModelError):
             harness.model.get_secret(id=secret_id)
 
     def test_set_user_secret_content(self, request: pytest.FixtureRequest):
@@ -5832,15 +5832,22 @@ def test_set_user_secret_content(self, request: pytest.FixtureRequest):
         secret = harness.model.get_secret(id=secret_id)
         assert secret.get_content(refresh=True) == {'password': 'bar'}
 
-    def test_get_user_secret_info(self, request: pytest.FixtureRequest):
-        harness = ops.testing.Harness(EventRecorder, meta=yaml.safe_dump({'name': 'webapp'}))
+    def test_user_secret_permissions(self, request: pytest.FixtureRequest):
+        harness = ops.testing.Harness(ops.CharmBase, meta='name: database')
         request.addfinalizer(harness.cleanup)
         harness.begin()
-        secret_id = harness.add_user_secret({'password': 'foo'})
-        harness.grant_secret(secret_id, 'webapp')
-        secret = harness.model.get_secret(id=secret_id)
-        with pytest.raises(ops.SecretNotFoundError):
+
+        # Charms can only view a user secret.
+        secret_id = harness.add_user_secret({'password': '1234'})
+        harness.grant_secret(secret_id, 'database')
+        secret = harness.charm.model.get_secret(id=secret_id)
+        assert secret.get_content() == {'password': '1234'}
+        with pytest.raises(ops.model.SecretNotFoundError):
             secret.get_info()
+        with pytest.raises(RuntimeError):
+            secret.set_content({'password': '5678'})
+        with pytest.raises(RuntimeError):
+            secret.remove_all_revisions()
 
 
 class EventRecorder(ops.CharmBase):