From 0c426cc066adb8f988edefd301fc4d4166b4055b Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Fri, 22 Mar 2024 08:17:03 +0000 Subject: [PATCH] Optimize SAML integration handling (#206) --- src/charm.py | 54 +++++++++++++++++----------------------- tests/unit/helpers.py | 19 ++++++-------- tests/unit/test_charm.py | 43 -------------------------------- 3 files changed, 31 insertions(+), 85 deletions(-) diff --git a/src/charm.py b/src/charm.py index 24658d77..ca0305d4 100755 --- a/src/charm.py +++ b/src/charm.py @@ -202,21 +202,9 @@ def _on_config_changed(self, _: HookEvent) -> None: """ self._configure_pod() - def _on_saml_data_available(self, event: SamlDataAvailableEvent) -> None: + def _on_saml_data_available(self, _: SamlDataAvailableEvent) -> None: """Handle SAML data available.""" - if self.unit.is_leader(): - # Utilizing the SHA1 hash is safe in this case, so a nosec ignore will be put in place. - fingerprint = hashlib.sha1( - base64.b64decode(event.certificates[0]) - ).hexdigest() # nosec - relation = self.model.get_relation(DEFAULT_RELATION_NAME) - # Will ignore union-attr since asserting the relation type will make bandit complain. - relation.data[self.app].update( # type: ignore[union-attr] - { - "fingerprint": fingerprint, - } - ) - self._on_config_changed(event) + self._configure_pod() def _on_rolling_restart(self, _: ops.EventBase) -> None: """Handle rolling restart event. @@ -316,27 +304,31 @@ def _get_saml_config(self) -> typing.Dict[str, typing.Any]: """Get SAML configuration. Returns: - Dictionary with the SAML configuration settings.. + Dictionary with the SAML configuration settings. """ + relation_data = self.saml.get_relation_data() + if relation_data is None: + return {} + saml_config = {} - relation = self.model.get_relation(DEFAULT_RELATION_NAME) - if ( - relation is not None - and relation.data[self.app] - and relation.app - and relation.data[relation.app] - ): - saml_config["DISCOURSE_SAML_TARGET_URL"] = relation.data[relation.app][ - "single_sign_on_service_redirect_url" - ] - saml_config["DISCOURSE_SAML_FULL_SCREEN_LOGIN"] = ( - "true" if self.config["force_saml_login"] else "false" - ) - fingerprint = relation.data[self.app].get("fingerprint") - if fingerprint: - saml_config["DISCOURSE_SAML_CERT_FINGERPRINT"] = fingerprint + sso_redirect_endpoint = [ + e + for e in relation_data.endpoints + if e.name == "SingleSignOnService" + and e.binding == "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" + ][0] + + saml_config["DISCOURSE_SAML_TARGET_URL"] = str(sso_redirect_endpoint.url) + certificate = relation_data.certificates[0] + # discourse needs SHA1 fingerprint + saml_config["DISCOURSE_SAML_CERT_FINGERPRINT"] = ( + hashlib.sha1(base64.b64decode(certificate)).digest().hex(":").upper() # nosec + ) + saml_config["DISCOURSE_SAML_FULL_SCREEN_LOGIN"] = ( + "true" if self.config["force_saml_login"] else "false" + ) saml_sync_groups = [ x.strip() for x in self.config["saml_sync_groups"].split(",") if x.strip() ] diff --git a/tests/unit/helpers.py b/tests/unit/helpers.py index 6cbc79fb..8b4ed657 100644 --- a/tests/unit/helpers.py +++ b/tests/unit/helpers.py @@ -17,7 +17,7 @@ def start_harness( *, - saml_fields: tuple = (False, "", ""), + saml_fields: tuple = (False, ""), with_postgres: bool = True, with_redis: bool = True, with_ingress: bool = False, @@ -53,7 +53,7 @@ def start_harness( _add_ingress_relation(harness) if saml_fields[0]: - _add_saml_relation(harness, saml_fields[1], saml_fields[2]) + _add_saml_relation(harness, saml_fields[1]) if with_config is not None: harness.update_config(with_config) @@ -136,7 +136,7 @@ def _add_ingress_relation(harness): harness.add_relation_unit(nginx_route_relation_id, "ingress/0") -def _add_saml_relation(harness, saml_target, fingerprint): +def _add_saml_relation(harness, saml_target): """Add ingress relation and relation data to the charm. Args: @@ -148,19 +148,16 @@ def _add_saml_relation(harness, saml_target, fingerprint): saml_relation_id = harness.add_relation("saml", "saml-integrator") harness.add_relation_unit(saml_relation_id, "saml-integrator/0") harness.disable_hooks() + binding = "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" harness.update_relation_data( relation_id=saml_relation_id, app_or_unit="saml-integrator", key_values={ + "entity_id": saml_target, + "metadata_url": f"{saml_target}/saml/metadata", + "x509certs": "test", "single_sign_on_service_redirect_url": f"{saml_target}/+saml", + "single_sign_on_service_redirect_binding": binding, }, ) harness.enable_hooks() - harness.update_relation_data( - relation_id=saml_relation_id, - app_or_unit=harness.charm.app.name, - key_values={ - "entity_id": saml_target, - "fingerprint": fingerprint, - }, - ) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index f3e2c48a..74b06ecc 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -216,49 +216,6 @@ def bundle_handler(args: ops.testing.ExecArgs) -> None: assert isinstance(harness.model.unit.status, ActiveStatus) -def test_on_config_changed_when_valid_no_fingerprint(): - """ - arrange: given a deployed discourse charm with all the required relations - act: when a valid configuration is provided - assert: the appropriate configuration values are passed to the pod and the unit - reaches Active status. - """ - harness = helpers.start_harness( - with_config={ - "force_saml_login": True, - "saml_sync_groups": "group1", - "s3_enabled": False, - "force_https": True, - }, - saml_fields=(True, "https://login.sample.com", ""), - ) - - harness.container_pebble_ready(SERVICE_NAME) - - updated_plan = harness.get_container_pebble_plan(SERVICE_NAME).to_dict() - updated_plan_env = updated_plan["services"][SERVICE_NAME]["environment"] - assert "*" == updated_plan_env["DISCOURSE_CORS_ORIGIN"] - assert "dbhost" == updated_plan_env["DISCOURSE_DB_HOST"] - assert DATABASE_NAME == updated_plan_env["DISCOURSE_DB_NAME"] - assert "somepasswd" == updated_plan_env["DISCOURSE_DB_PASSWORD"] - assert "someuser" == updated_plan_env["DISCOURSE_DB_USERNAME"] - assert updated_plan_env["DISCOURSE_ENABLE_CORS"] - assert "discourse-k8s" == updated_plan_env["DISCOURSE_HOSTNAME"] - assert "redis-host" == updated_plan_env["DISCOURSE_REDIS_HOST"] - assert "1010" == updated_plan_env["DISCOURSE_REDIS_PORT"] - assert "DISCOURSE_SAML_CERT_FINGERPRINT" not in updated_plan_env - assert "true" == updated_plan_env["DISCOURSE_SAML_FULL_SCREEN_LOGIN"] - assert "https://login.sample.com/+saml" == updated_plan_env["DISCOURSE_SAML_TARGET_URL"] - assert "false" == updated_plan_env["DISCOURSE_SAML_GROUPS_FULLSYNC"] - assert "true" == updated_plan_env["DISCOURSE_SAML_SYNC_GROUPS"] - assert "group1" == updated_plan_env["DISCOURSE_SAML_SYNC_GROUPS_LIST"] - assert updated_plan_env["DISCOURSE_SERVE_STATIC_ASSETS"] - assert "none" == updated_plan_env["DISCOURSE_SMTP_AUTHENTICATION"] - assert "none" == updated_plan_env["DISCOURSE_SMTP_OPENSSL_VERIFY_MODE"] - assert "DISCOURSE_USE_S3" not in updated_plan_env - assert isinstance(harness.model.unit.status, ActiveStatus) - - def test_on_config_changed_when_valid(): """ arrange: given a deployed discourse charm with all the required relations