Secret ownership fixes

README.md

@@ -191,10 +191,11 @@ def test_statuses():
         WaitingStatus('checking this is right...'),
         ActiveStatus("I am ruled"),
     ]
-
+
+    # similarly you can check the app status history:
     assert ctx.app_status_history == [
         UnknownStatus(),
-        ActiveStatus(""),
+        ...
     ]
 ```
 
@@ -434,9 +435,8 @@ that this unit can see).
 Because of that, `SubordinateRelation`, compared to `Relation`, always talks in terms of `remote`:
 
 - `Relation.remote_units_data` becomes `SubordinateRelation.remote_unit_data` taking a single `Dict[str:str]`. The remote unit ID can be provided as a separate argument. 
-- `Relation.remote_unit_ids` becomes `SubordinateRelation.primary_id` (a single ID instead of a list of IDs)
+- `Relation.remote_unit_ids` becomes `SubordinateRelation.remote_unit_id` (a single ID instead of a list of IDs)
 - `Relation.remote_units_data` becomes `SubordinateRelation.remote_unit_data` (a single databag instead of a mapping from unit IDs to databags)
-- `Relation.remote_app_name` maps to `SubordinateRelation.primary_app_name`
 
 ```python
 from scenario.state import SubordinateRelation
@@ -657,12 +657,12 @@ def test_pebble_push():
     state_in = State(
         containers=[container]
     )
-    Context(
+    ctx = Context(
         MyCharm,
-        meta={"name": "foo", "containers": {"foo": {}}}).run(
-        "start",
-        state_in,
+        meta={"name": "foo", "containers": {"foo": {}}}
     )
+
+    ctx.run("start", state_in)
 
     # this is the root of the simulated container filesystem. Any mounts will be symlinks in it.
     container_root_fs = container.get_filesystem(ctx)
@@ -831,11 +831,21 @@ state = State(
 ```
 
 The only mandatory arguments to Secret are its secret ID (which should be unique) and its 'contents': that is, a mapping
-from revision numbers (integers) to a str:str dict representing the payload of the revision.
+from revision numbers (integers) to a `str:str` dict representing the payload of the revision.
 
-By default, the secret is not owned by **this charm** nor is it granted to it.
-Therefore, if charm code attempted to get that secret revision, it would get a permission error: we didn't grant it to
-this charm, nor we specified that the secret is owned by it.
+There are three cases:
+- the secret is owned by this app but not this unit, in which case this charm can only manage it if we are the leader
+- the secret is owned by this unit, in which case this charm can always manage it (leader or not)
+- (default) the secret is not owned by this app nor unit, which means we can't manage it but only view it
+
+Thus by default, the secret is not owned by **this charm**, but, implicitly, by some unknown 'other charm', and that other charm has granted us view rights.
+
+
+The presence of the secret in `State.secrets` entails that we have access to it, either as owners or as grantees. Therefore, if we're not owners, we must be grantees. Absence of a Secret from the known secrets list means we are not entitled to obtaining it in any way. The charm, indeed, shouldn't even know it exists.
+
+[note]
+If this charm does not own the secret, but also it was not granted view rights by the (remote) owner, you model this in Scenario by _not adding it to State.secrets_! The presence of a `Secret` in `State.secrets` means, in other words, that the charm has view rights (otherwise, why would we put it there?). If the charm owns the secret, or is leader, it will _also_ have manage rights on top of view ones.
+[/note]
 
 To specify a secret owned by this unit (or app):
 
@@ -846,7 +856,7 @@ state = State(
     secrets=[
         Secret(
             id='foo',
-            contents={0: {'key': 'public'}},
+            contents={0: {'key': 'private'}},
             owner='unit',  # or 'app'
             remote_grants={0: {"remote"}}
             # the secret owner has granted access to the "remote" app over some relation with ID 0
@@ -866,7 +876,6 @@ state = State(
             id='foo',
             contents={0: {'key': 'public'}},
             # owner=None, which is the default
-            granted="unit",  # or "app",
             revision=0,  # the revision that this unit (or app) is currently tracking
         )
     ]

pyproject.toml

@@ -20,7 +20,6 @@ keywords = ["juju", "test"]
 dependencies = [
     "ops>=2.6",
     "PyYAML>=6.0.1",
-    "typer==0.7.0",
 ]
 readme = "README.md"
 requires-python = ">=3.8"
@@ -84,7 +83,7 @@ line-ending = "auto"
 [tool.pyright]
 ignore = [
     "scenario/sequences.py",
-    "scenario/cpature_events.py"
+    "scenario/capture_events.py"
 ]
 [tool.isort]
 profile = "black"

scenario/context.py

@@ -23,6 +23,8 @@
 
 logger = scenario_logger.getChild("runtime")
 
+DEFAULT_JUJU_VERSION = "3.4"
+
 
 @dataclasses.dataclass
 class ActionOutput:
@@ -130,10 +132,10 @@ def run(self) -> "State":
             return cast("State", super().run())
 
     def _runner(self):
-        return self._ctx._run_event
+        return self._ctx._run_event  # noqa
 
     def _get_output(self):
-        return self._ctx._output_state
+        return self._ctx._output_state  # noqa
 
 
 class _ActionManager(_Manager):
@@ -144,10 +146,10 @@ def run(self) -> "ActionOutput":
             return cast("ActionOutput", super().run())
 
     def _runner(self):
-        return self._ctx._run_action
+        return self._ctx._run_action  # noqa
 
     def _get_output(self):
-        return self._ctx._finalize_action(self._ctx.output_state)
+        return self._ctx._finalize_action(self._ctx.output_state)  # noqa
 
 
 class Context:
@@ -160,7 +162,7 @@ def __init__(
         actions: Optional[Dict[str, Any]] = None,
         config: Optional[Dict[str, Any]] = None,
         charm_root: Optional["PathLike"] = None,
-        juju_version: str = "3.0",
+        juju_version: str = DEFAULT_JUJU_VERSION,
         capture_deferred_events: bool = False,
         capture_framework_events: bool = False,
         app_name: Optional[str] = None,
@@ -197,7 +199,7 @@ def __init__(
 
         >>> from scenario import Context, State
         >>> from ops import ActiveStatus
-        >>> from charm import MyCharm, MyCustomEvent
+        >>> from charm import MyCharm, MyCustomEvent  # noqa
         >>>
         >>> def test_foo():
         >>>     # Arrange: set the context up
@@ -257,6 +259,11 @@ def __init__(
         self.charm_spec = spec
         self.charm_root = charm_root
         self.juju_version = juju_version
+        if juju_version.split(".")[0] == "2":
+            logger.warn(
+                "Juju 2.x is closed and unsupported. You may encounter inconsistencies.",
+            )
+
         self._app_name = app_name
         self._unit_id = unit_id
         self._tmp = tempfile.TemporaryDirectory()
@@ -364,7 +371,7 @@ def _coalesce_event(event: Union[str, Event]) -> Event:
         if not isinstance(event, Event):
             raise InvalidEventError(f"Expected Event | str, got {type(event)}")
 
-        if event._is_action_event:
+        if event._is_action_event:  # noqa
             raise InvalidEventError(
                 "Cannot Context.run() action events. "
                 "Use Context.run_action instead.",
@@ -394,9 +401,9 @@ def manager(
 
         Usage:
         >>> with Context().manager("start", State()) as manager:
-        >>>     assert manager.charm._some_private_attribute == "foo"
+        >>>     assert manager.charm._some_private_attribute == "foo"  # noqa
         >>>     manager.run()  # this will fire the event
-        >>>     assert manager.charm._some_private_attribute == "bar"
+        >>>     assert manager.charm._some_private_attribute == "bar"  # noqa
 
         :arg event: the Event that the charm will respond to. Can be a string or an Event instance.
         :arg state: the State instance to use as data source for the hook tool calls that the
@@ -413,9 +420,9 @@ def action_manager(
 
         Usage:
         >>> with Context().action_manager("foo-action", State()) as manager:
-        >>>     assert manager.charm._some_private_attribute == "foo"
+        >>>     assert manager.charm._some_private_attribute == "foo"  # noqa
         >>>     manager.run()  # this will fire the event
-        >>>     assert manager.charm._some_private_attribute == "bar"
+        >>>     assert manager.charm._some_private_attribute == "bar"  # noqa
 
         :arg action: the Action that the charm will execute. Can be a string or an Action instance.
         :arg state: the State instance to use as data source for the hook tool calls that the

scenario/mocking.py

@@ -52,6 +52,7 @@
         Event,
         ExecOutput,
         Relation,
+        Secret,
         State,
         SubordinateRelation,
         _CharmSpec,
@@ -167,6 +168,14 @@ def _get_relation_by_id(
             raise RelationNotFoundError()
 
     def _get_secret(self, id=None, label=None):
+        # FIXME: what error would a charm get IRL?
+        # ops 2.0 supports secrets, but juju only supports it from 3.0.2
+        if self._context.juju_version < "3.0.2":
+            raise RuntimeError(
+                "secrets are only available in juju >= 3.0.2."
+                "Set ``Context.juju_version`` to 3.0.2+ to use them.",
+            )
+
         canonicalize_id = Secret_Ops._canonicalize_id
 
         if id:
@@ -360,6 +369,23 @@ def secret_add(
         self._state.secrets.append(secret)
         return secret_id
 
+    def _check_can_manage_secret(
+        self,
+        secret: "Secret",
+    ):
+        if secret.owner is None:
+            raise SecretNotFoundError(
+                "this secret is not owned by this unit/app or granted to it. "
+                "Did you forget passing it to State.secrets?",
+            )
+        if secret.owner == "app" and not self.is_leader():
+            understandable_error = SecretNotFoundError(
+                f"App-owned secret {secret.id!r} can only be "
+                f"managed by the leader.",
+            )
+            # charm-facing side: respect ops error
+            raise ModelError("ERROR permission denied") from understandable_error
+
     def secret_get(
         self,
         *,
@@ -369,6 +395,14 @@ def secret_get(
         peek: bool = False,
     ) -> Dict[str, str]:
         secret = self._get_secret(id, label)
+        juju_version = self._context.juju_version
+        if not (juju_version == "3.1.7" or juju_version >= "3.3.1"):
+            # in this medieval juju chapter,
+            # secret owners always used to track the latest revision.
+            # ref: https://bugs.launchpad.net/juju/+bug/2037120
+            if secret.owner is not None:
+                refresh = True
+
         revision = secret.revision
         if peek or refresh:
             revision = max(secret.contents.keys())
@@ -384,8 +418,9 @@ def secret_info_get(
         label: Optional[str] = None,
     ) -> SecretInfo:
         secret = self._get_secret(id, label)
-        if not secret.owner:
-            raise RuntimeError(f"not the owner of {secret}")
+
+        # only "manage"=write access level can read secret info
+        self._check_can_manage_secret(secret)
 
         return SecretInfo(
             id=secret.id,
@@ -407,8 +442,7 @@ def secret_set(
         rotate: Optional[SecretRotate] = None,
     ):
         secret = self._get_secret(id, label)
-        if not secret.owner:
-            raise RuntimeError(f"not the owner of {secret}")
+        self._check_can_manage_secret(secret)
 
         secret._update_metadata(
             content=content,
@@ -420,8 +454,7 @@ def secret_set(
 
     def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None):
         secret = self._get_secret(id)
-        if not secret.owner:
-            raise RuntimeError(f"not the owner of {secret}")
+        self._check_can_manage_secret(secret)
 
         grantee = unit or self.relation_remote_app_name(
             relation_id,
@@ -435,8 +468,7 @@ 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):
         secret = self._get_secret(id)
-        if not secret.owner:
-            raise RuntimeError(f"not the owner of {secret}")
+        self._check_can_manage_secret(secret)
 
         grantee = unit or self.relation_remote_app_name(
             relation_id,
@@ -448,8 +480,7 @@ def secret_revoke(self, id: str, relation_id: int, *, unit: Optional[str] = None
 
     def secret_remove(self, id: str, *, revision: Optional[int] = None):
         secret = self._get_secret(id)
-        if not secret.owner:
-            raise RuntimeError(f"not the owner of {secret}")
+        self._check_can_manage_secret(secret)
 
         if revision:
             del secret.contents[revision]

scenario/state.py

@@ -6,6 +6,7 @@
 import datetime
 import inspect
 import re
+import warnings
 from collections import namedtuple
 from enum import Enum
 from itertools import chain
@@ -151,10 +152,12 @@ class Secret(_DCBase):
     contents: Dict[int, "RawSecretRevisionContents"]
 
     # indicates if the secret is owned by THIS unit, THIS app or some other app/unit.
-    owner: Literal["unit", "application", None] = None
+    # if None, the implication is that the secret has been granted to this unit.
+    owner: Literal["unit", "app", None] = None
 
-    # has this secret been granted to this unit/app or neither? Only applicable if NOT owner
-    granted: Literal["unit", "app", False] = False
+    # deprecated! if a secret is not granted to this unit, omit it from State.secrets altogether.
+    # this attribute will be removed in Scenario 7+
+    granted: Literal["unit", "app", False] = "<DEPRECATED>"
 
     # what revision is currently tracked by this charm. Only meaningful if owner=False
     revision: int = 0
@@ -168,6 +171,27 @@ class Secret(_DCBase):
     expire: Optional[datetime.datetime] = None
     rotate: Optional[SecretRotate] = None
 
+    def __post_init__(self):
+        if self.granted != "<DEPRECATED>":
+            msg = (
+                "``state.Secret.granted`` is deprecated and will be removed in Scenario 7+. "
+                "If a Secret is not owned by the app/unit you are testing, nor has been granted to "
+                "it by the (remote) owner, then omit it from ``State.secrets`` altogether."
+            )
+            logger.warning(msg)
+            warnings.warn(msg, DeprecationWarning, stacklevel=2)
+
+        if self.owner == "application":
+            msg = (
+                "Secret.owner='application' is deprecated in favour of 'app' "
+                "and will be removed in Scenario 7+."
+            )
+            logger.warning(msg)
+            warnings.warn(msg, DeprecationWarning, stacklevel=2)
+
+            # bypass frozen dataclass
+            object.__setattr__(self, "owner", "app")
+
     # consumer-only events
     @property
     def changed_event(self):
@@ -221,8 +245,9 @@ def _update_metadata(
     ):
         """Update the metadata."""
         revision = max(self.contents.keys())
+        self.contents[revision + 1] = content
+
         # bypass frozen dataclass
-        object.__setattr__(self, "contents"[revision + 1], content)
         if label:
             object.__setattr__(self, "label", label)
         if description:
@@ -856,10 +881,11 @@ class State(_DCBase):
     model: Model = Model()
     """The model this charm lives in."""
     secrets: List[Secret] = dataclasses.field(default_factory=list)
-    """The secrets this charm has access to (as an owner, or as a grantee)."""
+    """The secrets this charm has access to (as an owner, or as a grantee).
+    The presence of a secret in this list entails that the charm can read it.
+    Whether it can manage it or not depends on the individual secret's `owner` flag."""
     resources: Dict[str, "PathLike"] = dataclasses.field(default_factory=dict)
     """Mapping from resource name to path at which the resource can be found."""
-
     planned_units: int = 1
     """Number of non-dying planned units that are expected to be running this application.
     Use with caution."""