From 297c68c2996212ebf475bc3ffdf5ba580780cd71 Mon Sep 17 00:00:00 2001 From: Sebastian Neuser Date: Thu, 21 Nov 2024 20:53:01 +0100 Subject: [PATCH] impr: Introduce `one_of` list for SSO attribute values --- changelog.d/17949.feature | 2 +- .../configuration/config_documentation.md | 10 +++++---- synapse/config/oidc.py | 2 +- synapse/config/saml2.py | 2 +- synapse/config/sso.py | 22 ++++++++++++++++--- synapse/handlers/sso.py | 11 ++++++---- tests/handlers/test_oidc.py | 2 +- tests/handlers/test_saml.py | 2 +- 8 files changed, 37 insertions(+), 16 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/oidc.py b/synapse/config/oidc.py index d0a03baf55d..4966eb2cf15 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -283,7 +283,7 @@ def _parse_oidc_config_dict( ) # parse attribute_requirements from config (list of dicts) into a list of SsoAttributeRequirement attribute_requirements = [ - SsoAttributeRequirement(**x) + SsoAttributeRequirement.from_dict(x) for x in oidc_config.get("attribute_requirements", []) ] diff --git a/synapse/config/saml2.py b/synapse/config/saml2.py index 9d7ef94507a..df46c7a52c5 100644 --- a/synapse/config/saml2.py +++ b/synapse/config/saml2.py @@ -245,4 +245,4 @@ def _parse_attribute_requirements_def( attribute_requirements, config_path=("saml2_config", "attribute_requirements"), ) - return [SsoAttributeRequirement(**x) for x in attribute_requirements] + return [SsoAttributeRequirement.from_dict(x) for x in attribute_requirements] diff --git a/synapse/config/sso.py b/synapse/config/sso.py index d7a2187e7dc..e16840bf0bb 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, Self import attr @@ -45,13 +45,29 @@ class SsoAttributeRequirement: attribute: str # If a value is not given, than the attribute must simply exist. value: Optional[str] + one_of: Optional[List[str]] 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"]}, + #], } + @staticmethod + def from_dict(attr_req: Dict[str, Any]) -> Self: + attribute = attr_req["attribute"] + value = attr_req.get("value") + one_of = attr_req.get("one_of") + return SsoAttributeRequirement(attribute, value, one_of) + class SSOConfig(Config): """SSO Configuration""" 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..360cdbc42ff 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -1271,7 +1271,7 @@ 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"]}], } } ) diff --git a/tests/handlers/test_saml.py b/tests/handlers/test_saml.py index 7662869065e..e59d829489d 100644 --- a/tests/handlers/test_saml.py +++ b/tests/handlers/test_saml.py @@ -367,7 +367,7 @@ def test_attribute_requirements(self) -> None: { "saml2_config": { "attribute_requirements": [ - {"attribute": "userGroup", "value": "staff,admin"}, + {"attribute": "userGroup", "one_of": ["staff", "admin"]}, ], }, }