fix: adjust Harness secret behaviour to align with Juju (#1248)

Adjust Harness so that the errors match what Juju does in practice (I
tested each case - the results are in a table in a comment in #1229).
* When Juju responds with a message including "not found", we raise
`SecretNotFoundError`
* When Juju responds with any other message (such as "permission
denied"), we raise `ModelError`
* When Juju succeeds and only fails at the completion of the hook, we
raise `RuntimeError` - in production, the charm cannot catch this, but
we do want to surface this error, so `RuntimeError` seems most
approprate

In addition:
* Update the documentation to clarify when secret content is cached
(this was a request from the discussion with the data platform team in
Madrid)
* Correct the documentation regarding when `ModelError` and when
`SecretNotFoundError` will be raised with the secret methods
* Don't clear the `Secret` object local cache of the secret content on
`set_contents`. The contents won't change until `refresh` is used, so
there's no point forcing the next call to get the content from Juju.

Juju may change the responses so that there is increased consistency
(probably via [this bug](https://bugs.launchpad.net/juju/+bug/2067336))
but it seems best if we fix the behaviour to match now, and then if Juju
changes in the future, we can update ops then (also deciding at that
time how to handle the issue that different Juju versions will have
different behaviour).

Fixes #1229.

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):