From 8f07ef5c93baed5b1259ed9ce3ed1087b7a2d168 Mon Sep 17 00:00:00 2001
From: meise <meise@users.noreply.github.com>
Date: Mon, 10 Feb 2025 16:36:21 +0100
Subject: [PATCH] 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 | 1 +
.../configuration/config_documentation.md | 9 ++--
synapse/config/sso.py | 20 ++++++--
synapse/handlers/sso.py | 6 ++-
tests/handlers/test_oidc.py | 32 +++++++++++++
tests/handlers/test_saml.py | 46 +++++++++++++++++++
6 files changed, 105 insertions(+), 9 deletions(-)
create mode 100644 changelog.d/17949.feature
diff --git a/changelog.d/17949.feature b/changelog.d/17949.feature
new file mode 100644
index 0000000000..ee9bb51e03
--- /dev/null
+++ b/changelog.d/17949.feature
@@ -0,0 +1 @@
+Add functionality to be able to use multiple values in SSO feature `attribute_requirements`.
diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md
index a1e671ab8e..a5f23149ec 100644
--- a/docs/usage/configuration/config_documentation.md
+++ b/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'
```
diff --git a/synapse/config/sso.py b/synapse/config/sso.py
index d7a2187e7d..97b85e47ea 100644
--- a/synapse/config/sso.py
+++ b/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"]},
+ ],
}
diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py
index cee2eefbb3..531ed57110 100644
--- a/synapse/handlers/sso.py
+++ b/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')",
diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py
index a81501979d..1b43ee43c6 100644
--- a/tests/handlers/test_oidc.py
+++ b/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": {
diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py
index 6ab8fda6e7..1aca354826 100644
--- a/tests/handlers/test_saml.py
+++ b/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"""
--
GitLab