From 09aede6adb695fb248df89f605e5b7c1d1e18ec5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mei=C3=9Fner?= Date: Wed, 20 Nov 2024 18:01:13 +0100 Subject: [PATCH 1/3] feat: Allow multiple values for SSO attributes via comma separation --- changelog.d/17949.feature | 1 + .../configuration/config_documentation.md | 5 +- synapse/handlers/sso.py | 5 +- tests/handlers/test_oidc.py | 30 ++++++++++++ tests/handlers/test_saml.py | 46 +++++++++++++++++++ 5 files changed, 83 insertions(+), 4 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 00000000000..1ad9b42d01a --- /dev/null +++ b/changelog.d/17949.feature @@ -0,0 +1 @@ +Add functionality to be able to use multiple values in SSO featute `attribute_requirements` via comma seperated list. \ No newline at end of file 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""" From a38b58c72608c854faddfa8b98ab3b299a564410 Mon Sep 17 00:00:00 2001 From: Sebastian Neuser Date: Thu, 21 Nov 2024 20:53:01 +0100 Subject: [PATCH 2/3] impr: Introduce `one_of` list for SSO attribute values --- changelog.d/17949.feature | 2 +- .../usage/configuration/config_documentation.md | 10 ++++++---- synapse/config/sso.py | 17 +++++++++++++---- synapse/handlers/sso.py | 11 +++++++---- tests/handlers/test_oidc.py | 6 ++++-- tests/handlers/test_saml.py | 4 ++-- 6 files changed, 33 insertions(+), 17 deletions(-) diff --git a/changelog.d/17949.feature b/changelog.d/17949.feature index 1ad9b42d01a..ee9bb51e032 100644 --- a/changelog.d/17949.feature +++ b/changelog.d/17949.feature @@ -1 +1 @@ -Add functionality to be able to use multiple values in SSO featute `attribute_requirements` via comma seperated list. \ No newline at end of file +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 198b4c9221f..fe95bedc8bd 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3298,9 +3298,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. Values can be comma-seperated to allow - multiple values for one attribute. + `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. @@ -3381,7 +3381,9 @@ saml2_config: - attribute: userGroup value: "staff" - attribute: department - value: "sales,admins" + one_of: + - "sales" + - "admins" idp_entityid: 'https://our_idp/entityid' ``` diff --git a/synapse/config/sso.py b/synapse/config/sso.py index d7a2187e7dc..8d8ee1ad78e 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 @@ -44,12 +44,21 @@ class SsoAttributeRequirement: attribute: str # If a value is not given, than the attribute must simply exist. - value: Optional[str] + 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 1706eb6c9a4..81c00443fa2 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -1276,13 +1276,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] - for req_value in req.value.split(","): - if req_value in values: - return True + 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 2fd229cad61..1b43ee43c65 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -1271,11 +1271,13 @@ def test_attribute_requirements_contains(self) -> None: { "oidc_config": { **DEFAULT_CONFIG, - "attribute_requirements": [{"attribute": "test", "value": "foo,bar"}], + "attribute_requirements": [ + {"attribute": "test", "one_of": ["foo", "bar"]} + ], } } ) - def test_attribute_requirements_multiple_values(self) -> None: + 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 = { diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index 7662869065e..1aca3548267 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -367,12 +367,12 @@ def test_attribute_requirements(self) -> None: { "saml2_config": { "attribute_requirements": [ - {"attribute": "userGroup", "value": "staff,admin"}, + {"attribute": "userGroup", "one_of": ["staff", "admin"]}, ], }, } ) - def test_attribute_requirements_multiple_values(self) -> None: + def test_attribute_requirements_one_of(self) -> None: """The required attributes can be comma-separated.""" # stub out the auth handler From b5064069ca1726d304168f9b038e495a07cb2eec Mon Sep 17 00:00:00 2001 From: Sebastian Neuser Date: Wed, 27 Nov 2024 18:04:22 +0100 Subject: [PATCH 3/3] impr: Clarify comment on (sometimes) optional SSO config values --- synapse/config/sso.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/config/sso.py b/synapse/config/sso.py index 8d8ee1ad78e..97b85e47ea4 100644 --- a/synapse/config/sso.py +++ b/synapse/config/sso.py @@ -43,7 +43,8 @@ class SsoAttributeRequirement: """Object describing a single requirement for SSO attributes.""" attribute: str - # If a value is not given, than the attribute must simply exist. + # 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