From 37c81f68bb4f9a6bba5078ec49204ddd72cc2505 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mei=C3=9Fner?= <meise@infra.run>
Date: Wed, 20 Nov 2024 18:01:13 +0100
Subject: [PATCH] feat: Allow multiple values for SSO attributes via comma
 separating

---
 .../configuration/config_documentation.md     |  5 +-
 synapse/handlers/sso.py                       |  5 +-
 tests/handlers/test_oidc.py                   | 30 ++++++++++++
 tests/handlers/test_saml.py                   | 46 +++++++++++++++++++
 4 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md
index 7a48d76bbb1..198b4c9221f 100644
--- a/docs/usage/configuration/config_documentation.md
+++ b/docs/usage/configuration/config_documentation.md
@@ -3299,7 +3299,8 @@ This setting has the following sub-options:
 * `attribute_requirements`: It is possible to configure Synapse to only allow logins if SAML attributes
     match particular values. The requirements can be listed under
    `attribute_requirements` as shown in the example. All of the listed attributes must
-    match for the login to be permitted.
+    match for the login to be permitted. Values can be comma-seperated to allow
+    multiple values for one attribute.
 * `idp_entityid`: If the metadata XML contains multiple IdP entities then the `idp_entityid`
    option must be set to the entity to redirect users to.
    Most deployments only have a single IdP entity and so should omit this option.
@@ -3380,7 +3381,7 @@ saml2_config:
     - attribute: userGroup
       value: "staff"
     - attribute: department
-      value: "sales"
+      value: "sales,admins"
 
   idp_entityid: 'https://our_idp/entityid'
 ```
diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index ee74289b6c4..1706eb6c9a4 100644
--- a/synapse/handlers/sso.py
+++ b/synapse/handlers/sso.py
@@ -1280,8 +1280,9 @@ def _check_attribute_requirement(
         return True
 
     values = attributes[req.attribute]
-    if req.value in values:
-        return True
+    for req_value in req.value.split(","):
+        if req_value in values:
+            return True
 
     logger.info(
         "SSO attribute %s did not match required value '%s' (was '%s')",
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index a81501979d4..2fd229cad61 100644
--- a/tests/handlers/test_oidc.py
+++ b/tests/handlers/test_oidc.py
@@ -1267,6 +1267,36 @@ def test_attribute_requirements_contains(self) -> None:
             auth_provider_session_id=None,
         )
 
+    @override_config(
+        {
+            "oidc_config": {
+                **DEFAULT_CONFIG,
+                "attribute_requirements": [{"attribute": "test", "value": "foo,bar"}],
+            }
+        }
+    )
+    def test_attribute_requirements_multiple_values(self) -> None:
+        """Test that auth succeeds if userinfo attribute has multiple values and CONTAINS required value"""
+        # userinfo with "test": ["bar"] attribute should succeed.
+        userinfo = {
+            "sub": "tester",
+            "username": "tester",
+            "test": ["bar"],
+        }
+        request, _ = self.start_authorization(userinfo)
+        self.get_success(self.handler.handle_oidc_callback(request))
+
+        # check that the auth handler got called as expected
+        self.complete_sso_login.assert_called_once_with(
+            "@tester:test",
+            self.provider.idp_id,
+            request,
+            ANY,
+            None,
+            new_user=True,
+            auth_provider_session_id=None,
+        )
+
     @override_config(
         {
             "oidc_config": {
diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 6ab8fda6e73..7662869065e 100644
--- a/tests/handlers/test_saml.py
+++ b/tests/handlers/test_saml.py
@@ -363,6 +363,52 @@ def test_attribute_requirements(self) -> None:
             auth_provider_session_id=None,
         )
 
+    @override_config(
+        {
+            "saml2_config": {
+                "attribute_requirements": [
+                    {"attribute": "userGroup", "value": "staff,admin"},
+                ],
+            },
+        }
+    )
+    def test_attribute_requirements_multiple_values(self) -> None:
+        """The required attributes can be comma-separated."""
+
+        # stub out the auth handler
+        auth_handler = self.hs.get_auth_handler()
+        auth_handler.complete_sso_login = AsyncMock()  # type: ignore[method-assign]
+
+        # The response doesn't have the proper department.
+        saml_response = FakeAuthnResponse(
+            {"uid": "test_user", "username": "test_user", "userGroup": ["nogroup"]}
+        )
+        request = _mock_request()
+        self.get_success(
+            self.handler._handle_authn_response(request, saml_response, "redirect_uri")
+        )
+        auth_handler.complete_sso_login.assert_not_called()
+
+        # Add the proper attributes and it should succeed.
+        saml_response = FakeAuthnResponse(
+            {"uid": "test_user", "username": "test_user", "userGroup": ["admin"]}
+        )
+        request.reset_mock()
+        self.get_success(
+            self.handler._handle_authn_response(request, saml_response, "redirect_uri")
+        )
+
+        # check that the auth handler got called as expected
+        auth_handler.complete_sso_login.assert_called_once_with(
+            "@test_user:test",
+            "saml",
+            request,
+            "redirect_uri",
+            None,
+            new_user=True,
+            auth_provider_session_id=None,
+        )
+
 
 def _mock_request() -> Mock:
     """Returns a mock which will stand in as a SynapseRequest"""