diff --git a/ops/charm.py b/ops/charm.py index e083bd43b..9a1d79a12 100644 --- a/ops/charm.py +++ b/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() ` 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): diff --git a/ops/framework.py b/ops/framework.py index 661c6959d..e5caf0d8c 100644 --- a/ops/framework.py +++ b/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: diff --git a/ops/model.py b/ops/model.py index d87b022dd..040ba09c6 100644 --- a/ops/model.py +++ b/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,6 +1371,10 @@ 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. @@ -1372,6 +1382,11 @@ def get_content(self, *, refresh: bool = False) -> Dict[str, str]: 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,7 +1396,13 @@ 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) @@ -1389,6 +1410,10 @@ 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 `. + 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 ` or :class:`ops.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 `. + + 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 diff --git a/ops/testing.py b/ops/testing.py index a88686e47..22c4a5492 100644 --- a/ops/testing.py +++ b/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] diff --git a/test/test_model.py b/test/test_model.py index 5c9435b12..b0e4f6206 100644 --- a/test/test_model.py +++ b/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"}'""") diff --git a/test/test_testing.py b/test/test_testing.py index 963219b39..e75a8ade6 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -6002,9 +6002,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): @@ -6025,7 +6025,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): @@ -6037,7 +6037,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): @@ -6052,15 +6052,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):