diff --git a/interfaces/oauth/CHANGELOG.md b/interfaces/oauth/CHANGELOG.md index 4a6efc563..6849c5fc6 100644 --- a/interfaces/oauth/CHANGELOG.md +++ b/interfaces/oauth/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## 1.1.0 + +- Backport holistic client reconciliation fixes from `charms.hydra.v0.oauth`: + - Refresh client secret on provider info lookup to prevent stale revisions when secrets rotate. + - Handle invalid or incomplete relation databags gracefully without wedging hook execution. + - Support updating existing Juju secrets in `OAuthProvider._create_juju_secret`. + - Add `OAuthProvider.get_client_secret()` and `OAuthProvider.get_client_config()` methods. + ## 1.0.0 Initial release. Migrated from `charms.hydra.v0.oauth` (v0.12). diff --git a/interfaces/oauth/src/charmlibs/interfaces/oauth/__init__.py b/interfaces/oauth/src/charmlibs/interfaces/oauth/__init__.py index a002239a1..a1f1a3f51 100644 --- a/interfaces/oauth/src/charmlibs/interfaces/oauth/__init__.py +++ b/interfaces/oauth/src/charmlibs/interfaces/oauth/__init__.py @@ -61,6 +61,19 @@ def _set_client_config(self): OAUTH_GRANT_TYPES, ) self.oauth.update_client_config(client_config) + + +Provider +-------- + +Besides the ``client_created``/``client_changed`` events, a provider can read a requirer's +published configuration at any time with ``OAuthProvider.get_client_config(relation)``. It +returns ``None`` when the requirer has published nothing yet and raises ``DataValidationError`` +when what it published does not match the requirer schema. Use it to reconcile registered +clients holistically rather than relying on an event having been delivered. + +Note that ``client_created``/``client_changed`` are not emitted when the requirer's data fails +validation; the failure is logged and the relation is skipped rather than erroring the hook. """ from ._oauth import ( diff --git a/interfaces/oauth/src/charmlibs/interfaces/oauth/_oauth.py b/interfaces/oauth/src/charmlibs/interfaces/oauth/_oauth.py index c83162a3d..143d61682 100644 --- a/interfaces/oauth/src/charmlibs/interfaces/oauth/_oauth.py +++ b/interfaces/oauth/src/charmlibs/interfaces/oauth/_oauth.py @@ -500,7 +500,9 @@ def get_provider_info(self, relation_id: int | None = None) -> OauthProviderConf client_secret_id = cast('str | None', data.get('client_secret_id')) if client_secret_id: client_secret_obj = self.get_client_secret(client_secret_id) - client_secret = client_secret_obj.get_content()[CLIENT_SECRET_FIELD] + # `refresh=True`: the provider cuts a new revision when it rotates the secret, + # and nothing here observes `secret-changed`, so the tracked revision goes stale. + client_secret = client_secret_obj.get_content(refresh=True)[CLIENT_SECRET_FIELD] data['client_secret'] = client_secret oauth_provider = OauthProviderConfig.from_dict(data) @@ -688,7 +690,12 @@ def _get_client_config_from_relation_data(self, event: RelationChangedEvent) -> logger.info('No requirer relation data available.') return - client_data = _load_data(raw_data, OAUTH_REQUIRER_JSON_SCHEMA) + try: + client_data = _load_data(raw_data, OAUTH_REQUIRER_JSON_SCHEMA) + except DataValidationError: + logger.warning('The requirer relation data is not valid yet.') + return + redirect_uri = cast('str | None', client_data.get('redirect_uri')) scope = cast('str | None', client_data.get('scope')) grant_types = cast('list[str] | None', client_data.get('grant_types')) @@ -701,7 +708,15 @@ def _get_client_config_from_relation_data(self, event: RelationChangedEvent) -> if not provider_data_raw: logger.info('No provider relation data available.') return - provider_data = _load_data(provider_data_raw, OAUTH_PROVIDER_JSON_SCHEMA) + + try: + provider_data = _load_data(provider_data_raw, OAUTH_PROVIDER_JSON_SCHEMA) + except DataValidationError: + # A partially written provider databag must not wedge the hook: the charm can + # only repair it from a later hook, which would never run. + logger.warning('The provider relation data is not complete yet.') + return + client_id = cast('str | None', provider_data.get('client_id')) relation_id = event.relation.id @@ -737,9 +752,15 @@ def _on_relation_broken(self, event: RelationBrokenEvent) -> None: self.on.client_deleted.emit(event.relation.id) def _create_juju_secret(self, client_secret: str, relation: Relation) -> Secret: - """Create a juju secret and grant it to a relation.""" - secret = {CLIENT_SECRET_FIELD: client_secret} - juju_secret = self.model.app.add_secret(secret, label=self._get_secret_label(relation)) + """Create or update a juju secret and grant it to a relation.""" + content = {CLIENT_SECRET_FIELD: client_secret} + label = self._get_secret_label(relation) + try: + juju_secret = self.model.get_secret(label=label) + except SecretNotFoundError: + juju_secret = self.model.app.add_secret(content, label=label) + else: + juju_secret.set_content(content) juju_secret.grant(relation) return juju_secret @@ -754,6 +775,43 @@ def _delete_juju_secret(self, relation: Relation) -> None: def remove_secret(self, relation: Relation) -> None: return self._delete_juju_secret(relation) + def get_client_secret(self, relation: Relation) -> str | None: + """Return the client secret currently shared with the requirer, if there is one. + + Re-registering a client with this value keeps the requirer working: writing a + different secret would cut a new revision that the requirer does not track. + """ + try: + secret = self.model.get_secret(label=self._get_secret_label(relation)) + except SecretNotFoundError: + return None + + return secret.get_content(refresh=True).get(CLIENT_SECRET_FIELD) + + def get_client_config(self, relation: Relation) -> ClientConfig | None: + """Read the requirer's client configuration from the integration databag. + + Returns None when the requirer has not published its configuration yet. + + Raises: + DataValidationError: if the published data does not match the requirer schema. + """ + if not relation.app: + return None + + data = relation.data[relation.app] + if not data: + return None + + client_data = _load_data(data, OAUTH_REQUIRER_JSON_SCHEMA) + return ClientConfig( + redirect_uri=client_data.get('redirect_uri'), + scope=client_data['scope'], + grant_types=client_data['grant_types'], + audience=client_data['audience'], + token_endpoint_auth_method=client_data['token_endpoint_auth_method'], + ) + def set_provider_info_in_relation_data( self, issuer_url: str, @@ -799,8 +857,6 @@ def set_client_credentials_in_relation_data( relation = self.model.get_relation(self._relation_name, relation_id) if not relation or not relation.app: return - # TODO: What if we are refreshing the client_secret? We need to add a - # new revision for that secret = self._create_juju_secret(client_secret, relation) data = {'client_id': client_id, 'client_secret_id': secret.id} relation.data[self.model.app].update(_dump_data(data)) diff --git a/interfaces/oauth/src/charmlibs/interfaces/oauth/_version.py b/interfaces/oauth/src/charmlibs/interfaces/oauth/_version.py index a5443891f..f9e828ee1 100644 --- a/interfaces/oauth/src/charmlibs/interfaces/oauth/_version.py +++ b/interfaces/oauth/src/charmlibs/interfaces/oauth/_version.py @@ -12,4 +12,4 @@ # See the License for the specific language governing permissions and # limitations under the License. -__version__ = '1.0.0' +__version__ = '1.1.0' diff --git a/interfaces/oauth/tests/unit/test_oauth_provider.py b/interfaces/oauth/tests/unit/test_oauth_provider.py index e7a3a01b9..9c13f8e24 100644 --- a/interfaces/oauth/tests/unit/test_oauth_provider.py +++ b/interfaces/oauth/tests/unit/test_oauth_provider.py @@ -12,8 +12,10 @@ from charmlibs.interfaces.oauth import ( ClientChangedEvent, + ClientConfig, ClientCreatedEvent, ClientDeletedEvent, + DataValidationError, OAuthProvider, ) from charmlibs.interfaces.oauth._oauth import CLIENT_SECRET_FIELD @@ -68,6 +70,16 @@ def _on_relation_created(self, _: RelationCreatedEvent) -> None: scope='openid profile email phone', ) + def get_client_config(self, relation_id: int) -> ClientConfig | None: + rel = self.model.get_relation('oauth', relation_id) + assert rel is not None + return self.oauth.get_client_config(rel) + + def get_client_secret(self, relation_id: int) -> str | None: + rel = self.model.get_relation('oauth', relation_id) + assert rel is not None + return self.oauth.get_client_secret(rel) + def test_provider_info_in_relation_databag(context: Context[OAuthProviderCharm]) -> None: relation = Relation('oauth') @@ -224,3 +236,86 @@ def test_secret_removed_when_relation_removed(context: Context[OAuthProviderChar found_secret = next((s for s in state_out.secrets if s.id == secret_id), None) assert found_secret is None + + +def test_get_client_config(context: Context[OAuthProviderCharm]) -> None: + requirer_data = { + 'redirect_uri': 'https://oidc-client.com/callback', + 'scope': 'openid email', + 'grant_types': '["authorization_code"]', + 'audience': '["app1"]', + 'token_endpoint_auth_method': 'client_secret_basic', + } + relation = Relation('oauth', remote_app_data=requirer_data) + state = create_state(leader=True, relations=[relation]) + + with context(context.on.relation_changed(relation), state) as mgr: + mgr.run() + config = mgr.charm.get_client_config(relation.id) + assert config == ClientConfig( + redirect_uri='https://oidc-client.com/callback', + scope='openid email', + grant_types=['authorization_code'], + audience=['app1'], + token_endpoint_auth_method='client_secret_basic', + ) + + +def test_get_client_config_invalid_data(context: Context[OAuthProviderCharm]) -> None: + requirer_data = { + 'redirect_uri': 'https://oidc-client.com/callback', + 'scope': 'openid email', + 'grant_types': 'invalid_json', + } + relation = Relation('oauth', remote_app_data=requirer_data) + state = create_state(leader=True, relations=[relation]) + + with context(context.on.relation_changed(relation), state) as mgr: + mgr.run() + with pytest.raises(DataValidationError): + mgr.charm.get_client_config(relation.id) + + +def test_get_client_secret(context: Context[OAuthProviderCharm]) -> None: + relation = Relation('oauth') + secret = Secret( + owner='app', + label=f'client_secret_{relation.id}', + tracked_content={CLIENT_SECRET_FIELD: 'secret_val'}, + ) + state = create_state(leader=True, relations=[relation], secrets=[secret]) + + with context(context.on.relation_changed(relation), state) as mgr: + mgr.run() + secret_val = mgr.charm.get_client_secret(relation.id) + assert secret_val == 'secret_val' + + +def test_update_existing_secret_content(context: Context[OAuthProviderCharm]) -> None: + relation = Relation('oauth') + secret = Secret( + owner='app', + label=f'client_secret_{relation.id}', + tracked_content={CLIENT_SECRET_FIELD: 'old_secret'}, + ) + state = create_state(leader=True, relations=[relation], secrets=[secret]) + + with context(context.on.start(), state) as mgr: + mgr.run() + rel = mgr.charm.model.get_relation('oauth', relation.id) + assert rel is not None + mgr.charm.oauth.set_client_credentials_in_relation_data( + relation.id, 'new_client_id', 'new_secret_val' + ) + assert mgr.charm.oauth.get_client_secret(rel) == 'new_secret_val' + + +def test_get_client_config_from_relation_data_handles_invalid_data( + context: Context[OAuthProviderCharm], +) -> None: + requirer_data = {'invalid': 'data'} + relation = Relation('oauth', remote_app_data=requirer_data) + state = create_state(leader=True, relations=[relation]) + + context.run(context.on.relation_changed(relation), state) + assert not any(isinstance(e, ClientCreatedEvent) for e in context.emitted_events)