diff --git a/changelog.d/17949.feature b/changelog.d/17949.feature new file mode 100644 index 0000000000..1ad9b42d01 --- /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 7a48d76bbb..198b4c9221 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 ee74289b6c..1706eb6c9a 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 a81501979d..2fd229cad6 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -1267,6 +1267,36 @@ class OidcHandlerTestCase(HomeserverTestCase): 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 6ab8fda6e7..7662869065 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", "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"""