Optimize SAML integration handling (#206)

canonical · Mar 22, 2024 · 0c426cc · 0c426cc
1 parent e7ecf68
commit 0c426cc
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 85 deletions.
diff --git 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
@@ -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
@@ -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