feat: Allow multiple values for SSO attribute_requirements via comma separation (#17949)

In the current `attribute_requirements` implementation it is only
possible to allow exact matching attribute values. Multiple allowed
values for one attribute are not possible as described in #13238.

### Pull Request Checklist

<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->

* [x] Pull request is based on the develop branch
* [x] Pull request includes a [changelog
file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog).
The entry should:
- Be a short description of your change which makes sense to users.
"Fixed a bug that prevented receiving messages from other servers."
instead of "Moved X method from `EventStore` to `EventWorkerStore`.".
  - Use markdown where necessary, mostly for `code blocks`.
  - End with either a period (.) or an exclamation mark (!).
  - Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by
@github_username." or "Contributed by [Your Name]." to the end of the
entry.
* [x] [Code
style](https://element-hq.github.io/synapse/latest/code_style.html) is
correct
(run the
[linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))

---------

Co-authored-by: Sebastian Neuser <pzkz@infra.run>
Co-authored-by: Quentin Gliech <quenting@element.io>

changelog.d/17949.feature

@@ -0,0 +1 @@
+Add functionality to be able to use multiple values in SSO feature `attribute_requirements`.

docs/usage/configuration/config_documentation.md

@@ -3337,8 +3337,9 @@ This setting has the following sub-options:
    The default is 'uid'.
 * `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.
+    `attribute_requirements` as shown in the example. All of the listed attributes must
+    match for the login to be permitted. Values can be specified in a `one_of` list to allow
+    multiple values for an 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.
@@ -3419,7 +3420,9 @@ saml2_config:
     - attribute: userGroup
       value: "staff"
     - attribute: department
-      value: "sales"
+      one_of:
+        - "sales"
+        - "admins"
 
   idp_entityid: 'https://our_idp/entityid'
 ```

synapse/config/sso.py

@@ -19,7 +19,7 @@
 #
 #
 import logging
-from typing import Any, Dict, Optional
+from typing import Any, Dict, List, Optional
 
 import attr
 
@@ -43,13 +43,23 @@ class SsoAttributeRequirement:
     """Object describing a single requirement for SSO attributes."""
 
     attribute: str
-    # If a value is not given, than the attribute must simply exist.
-    value: Optional[str]
+    # If neither value nor one_of is given, the attribute must simply exist. This is
+    # only true for CAS configs which use a different JSON schema than the one below.
+    value: Optional[str] = None
+    one_of: Optional[List[str]] = None
 
     JSON_SCHEMA = {
         "type": "object",
-        "properties": {"attribute": {"type": "string"}, "value": {"type": "string"}},
-        "required": ["attribute", "value"],
+        "properties": {
+            "attribute": {"type": "string"},
+            "value": {"type": "string"},
+            "one_of": {"type": "array", "items": {"type": "string"}},
+        },
+        "required": ["attribute"],
+        "oneOf": [
+            {"required": ["value"]},
+            {"required": ["one_of"]},
+        ],
     }
 
 

synapse/handlers/sso.py

@@ -1277,12 +1277,16 @@ def _check_attribute_requirement(
         return False
 
     # If the requirement is None, the attribute existing is enough.
-    if req.value is None:
+    if req.value is None and req.one_of is None:
         return True
 
     values = attributes[req.attribute]
     if req.value in values:
         return True
+    if req.one_of:
+        for value in req.one_of:
+            if value in values:
+                return True
 
     logger.info(
         "SSO attribute %s did not match required value '%s' (was '%s')",

tests/handlers/test_oidc.py

@@ -1267,6 +1267,38 @@ class OidcHandlerTestCase(HomeserverTestCase):
             auth_provider_session_id=None,
         )
 
+    @override_config(
+        {
+            "oidc_config": {
+                **DEFAULT_CONFIG,
+                "attribute_requirements": [
+                    {"attribute": "test", "one_of": ["foo", "bar"]}
+                ],
+            }
+        }
+    )
+    def test_attribute_requirements_one_of(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": {

tests/handlers/test_saml.py

@@ -363,6 +363,52 @@ class SamlHandlerTestCase(HomeserverTestCase):
             auth_provider_session_id=None,
         )
 
+    @override_config(
+        {
+            "saml2_config": {
+                "attribute_requirements": [
+                    {"attribute": "userGroup", "one_of": ["staff", "admin"]},
+                ],
+            },
+        }
+    )
+    def test_attribute_requirements_one_of(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"""