feat: expand the secret ID out to the full URI when only given the ID (…

…canonical#1358)

Adjust the rules for getting the 'canonical' form of the Secret URI:
* If the passed ID starts with `secret:`, leave it as-is (unchanged)
* Otherwise, if the model UUID is unknown, add the `secret:` prefix to
end up with the form `secret:id` (unchanged)
* Otherwise, change to the full `secret://{model UUID}/{secret ID` form
(new)

Also adjust the code that calls the canonicalization [sic] method so
that the model UUID is always available in regular ops use. If someone
is creating `SecretInfo` objects themselves then a `None` value for the
UUID is the default, for backwards compatibility (but a warning is
issued in that case).

Fixes canonical#1312.

ops/model.py

@@ -30,6 +30,7 @@
 import tempfile
 import time
 import typing
+import warnings
 import weakref
 from abc import ABC, abstractmethod
 from pathlib import Path, PurePath
@@ -295,7 +296,7 @@ def get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) -
             raise TypeError('Must provide an id or label, or both')
         if id is not None:
             # Canonicalize to "secret:<id>" form for consistency in backend calls.
-            id = Secret._canonicalize_id(id)
+            id = Secret._canonicalize_id(id, self.uuid)
         content = self._backend.secret_get(id=id, label=label)
         return Secret(self._backend, id=id, label=label, content=content)
 
@@ -1182,8 +1183,17 @@ def __init__(
         rotation: Optional[SecretRotate],
         rotates: Optional[datetime.datetime],
         description: Optional[str] = None,
+        *,
+        model_uuid: Optional[str] = None,
     ):
-        self.id = Secret._canonicalize_id(id)
+        if model_uuid is None:
+            warnings.warn(
+                '`model_uuid` should always be provided when creating a '
+                'SecretInfo instance, and will be required in the future.',
+                DeprecationWarning,
+                stacklevel=2,
+            )
+        self.id = Secret._canonicalize_id(id, model_uuid)
         self.label = label
         self.revision = revision
         self.expires = expires
@@ -1192,7 +1202,9 @@ def __init__(
         self.description = description
 
     @classmethod
-    def from_dict(cls, id: str, d: Dict[str, Any]) -> 'SecretInfo':
+    def from_dict(
+        cls, id: str, d: Dict[str, Any], model_uuid: Optional[str] = None
+    ) -> 'SecretInfo':
         """Create new SecretInfo object from ID and dict parsed from JSON."""
         expires = typing.cast(Optional[str], d.get('expiry'))
         try:
@@ -1208,6 +1220,7 @@ def from_dict(cls, id: str, d: Dict[str, Any]) -> 'SecretInfo':
             rotation=rotation,
             rotates=timeconv.parse_rfc3339(rotates) if rotates is not None else None,
             description=typing.cast(Optional[str], d.get('description')),
+            model_uuid=model_uuid,
         )
 
     def __repr__(self):
@@ -1249,7 +1262,7 @@ def __init__(
         if not (id or label):
             raise TypeError('Must provide an id or label, or both')
         if id is not None:
-            id = self._canonicalize_id(id)
+            id = self._canonicalize_id(id, backend.model_uuid)
         self._backend = backend
         self._id = id
         self._label = label
@@ -1264,11 +1277,13 @@ def __repr__(self):
         return f"<Secret {' '.join(fields)}>"
 
     @staticmethod
-    def _canonicalize_id(id: str) -> str:
+    def _canonicalize_id(id: str, model_uuid: Optional[str]) -> str:
         """Return the canonical form of the given secret ID, with the 'secret:' prefix."""
         id = id.strip()
         if not id.startswith('secret:'):
-            id = f'secret:{id}'  # add the prefix if not there already
+            # Add the prefix and, if provided, model UUID.
+            id = f'secret:{id}' if model_uuid is None else f'secret://{model_uuid}/{id}'
+
         return id
 
     @classmethod
@@ -1308,8 +1323,8 @@ def id(self) -> Optional[str]:
         """Locator ID (URI) for this secret.
 
         This has an unfortunate name for historical reasons, as it's not
-        really a unique identifier, but the secret's locator URI, which may or
-        may not include the model UUID (for cross-model secrets).
+        really a unique identifier, but the secret's locator URI, which will
+        include the model UUID (for cross-model secrets).
 
         Charms should treat this as an opaque string for looking up secrets
         and sharing them via relation data. If a charm-local "name" is needed
@@ -3604,7 +3619,9 @@ def secret_info_get(
         result = self._run_for_secret('secret-info-get', *args, return_output=True, use_json=True)
         info_dicts = typing.cast(Dict[str, Any], result)
         id = list(info_dicts)[0]  # Juju returns dict of {secret_id: {info}}
-        return SecretInfo.from_dict(id, typing.cast(Dict[str, Any], info_dicts[id]))
+        return SecretInfo.from_dict(
+            id, typing.cast(Dict[str, Any], info_dicts[id]), model_uuid=self.model_uuid
+        )
 
     def secret_set(
         self,

ops/testing.py

@@ -24,6 +24,7 @@
 import os
 import pathlib
 import random
+import re
 import shutil
 import signal
 import tempfile
@@ -2702,7 +2703,7 @@ def planned_units(self) -> int:
         return len(units) + 1  # Account for this unit.
 
     def _get_secret(self, id: str) -> Optional[_Secret]:
-        return next((s for s in self._secrets if s.id == id), None)
+        return next((s for s in self._secrets if self._secret_ids_are_equal(s.id, id)), None)
 
     def _ensure_secret(self, id: str) -> _Secret:
         secret = self._get_secret(id)
@@ -2724,6 +2725,22 @@ def _ensure_secret_id_or_label(self, id: Optional[str], label: Optional[str]):
             )
         return secret
 
+    def _secret_ids_are_equal(self, id1: str, id2: str) -> bool:
+        secret_re = re.compile(
+            r'^(?:secret:)?(?://)?(?:(?P<uuid>[a-z0-9-]+)/)?(?P<id>[a-z0-9-]+)$', re.IGNORECASE
+        )
+        mo = secret_re.match(id1)
+        if not mo:
+            return False
+        model_uuid1 = mo.group('uuid') or self.model_uuid
+        id1 = mo.group('id')
+        mo = secret_re.match(id2)
+        if not mo:
+            return False
+        model_uuid2 = mo.group('uuid') or self.model_uuid
+        id2 = mo.group('id')
+        return model_uuid1 == model_uuid2 and id1 == id2
+
     def secret_get(
         self,
         *,
@@ -2816,6 +2833,7 @@ def secret_info_get(
             rotation=rotation,
             rotates=rotates,
             description=secret.description,
+            model_uuid=self.model_uuid,
         )
 
     def secret_set(
@@ -2899,7 +2917,7 @@ def _secret_add(
             description=description,
         )
         self._secrets.append(secret)
-        return id
+        return id  # Note that this is the 'short' ID, not the canonicalised one.
 
     def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None) -> None:
         secret = self._ensure_secret(id)
@@ -2936,9 +2954,11 @@ def secret_remove(self, id: str, *, revision: Optional[int] = None) -> None:
                 secret.revisions = revisions
             else:
                 # Last revision removed, remove entire secret
-                self._secrets = [s for s in self._secrets if s.id != id]
+                self._secrets = [
+                    s for s in self._secrets if not self._secret_ids_are_equal(s.id, id)
+                ]
         else:
-            self._secrets = [s for s in self._secrets if s.id != id]
+            self._secrets = [s for s in self._secrets if not self._secret_ids_are_equal(s.id, id)]
 
     def open_port(self, protocol: str, port: Optional[int] = None):
         self._check_protocol_and_port(protocol, port)