From 7711034b7f5ae95fe6b08945886d599da03cd06f Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 20:53:46 +0200 Subject: [PATCH 01/18] Add attribute_requirements to OIDC login SSO handling This way, administrators may require certain attributes be part of OIDC userinfo in order to allow login/registration using the OIDC SSO Provider. This lets one limit SSO logins to certain groups, by adding group memberships to OIDC userinfo - in Keycloak, this can be done with a Group Membership mapper. --- synapse/config/oidc_config.py | 32 +++++++++++++++++++++++++++++++- synapse/handlers/oidc_handler.py | 10 ++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 7f5e449eb298..6e918787f1db 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -15,10 +15,11 @@ # limitations under the License. from collections import Counter -from typing import Iterable, Mapping, Optional, Tuple, Type +from typing import Any, Iterable, List, Mapping, Optional, Tuple, Type import attr +from synapse.config.sso import SsoAttributeRequirement from synapse.config._util import validate_config from synapse.python_dependencies import DependencyException, check_requirements from synapse.types import Collection, JsonDict @@ -190,6 +191,19 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # When rendering, the Jinja2 templates are given a 'user' variable, # which is set to the claims returned by the UserInfo Endpoint and/or # in the ID Token. + # It is possible to configure Synapse to only allow logins if certain attributes + # match particular values in the OIDC userinfo. The requirements can be listed under + # `attribute_requirements` as shown below. All of the listed attributes must + # match for the login to be permitted. Note that fields listed may need to be added + # to the userinfo return in the OIDC provider, by extending or adding a scope. + # If the attributes listed are not part of the default scopes, these scopes may + # also need to be added to the `scopes` section of the OIDC config. + # + # attribute_requirements: + # - attribute: family_name + # value: "Stephensson" + # - attribute: groups + # value: "admin" # # See https://github.com/matrix-org/synapse/blob/master/docs/openid.md # for information on how to configure these options. @@ -223,6 +237,9 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # localpart_template: "{{{{ user.login }}}}" # display_name_template: "{{{{ user.name }}}}" # email_template: "{{{{ user.email }}}}" + # attribute_requirements: + # - attribute: userGroup + # value: "synapseUsers" # For use with Keycloak # @@ -232,6 +249,9 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # client_id: "synapse" # client_secret: "copy secret generated in Keycloak UI" # scopes: ["openid", "profile"] + # attribute_requirements: + # - attribute: groups + # value: "admin" # For use with Github # @@ -324,6 +344,10 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): }, "allow_existing_users": {"type": "boolean"}, "user_mapping_provider": {"type": ["object", "null"]}, + "attribute_requirements": { + "type": "array", + "items": SsoAttributeRequirement.JSON_SCHEMA + }, }, } @@ -460,6 +484,8 @@ def _parse_oidc_config_dict( jwt_header=client_secret_jwt_key_config["jwt_header"], jwt_payload=client_secret_jwt_key_config.get("jwt_payload", {}), ) + # parse attribute_requirements from config (list of dicts) into a list of SsoAttributeRequirement + attribute_requirements = [SsoAttributeRequirement(**x) for x in oidc_config.get("attribute_requirements", [])] return OidcProviderConfig( idp_id=idp_id, @@ -482,6 +508,7 @@ def _parse_oidc_config_dict( allow_existing_users=oidc_config.get("allow_existing_users", False), user_mapping_provider_class=user_mapping_provider_class, user_mapping_provider_config=user_mapping_provider_config, + attribute_requirements=attribute_requirements, ) @@ -568,3 +595,6 @@ class OidcProviderConfig: # the config of the user mapping provider user_mapping_provider_config = attr.ib() + + # required attributes to require in userinfo to allow login/registration + attribute_requirements = attr.ib(type=Optional[list]) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 825fadb76f9a..1a1f6586c024 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -278,6 +278,7 @@ def __init__( self._config = provider self._callback_url = hs.config.oidc_callback_url # type: str + self._oidc_attribute_requirements = provider.attribute_requirements # type: List[SsoAttributeRequirement] self._scopes = provider.scopes self._user_profile_method = provider.user_profile_method @@ -854,6 +855,15 @@ async def handle_oidc_callback( ) # otherwise, it's a login + logger.debug(f"Userinfo for OIDC login: {userinfo}") + + # Ensure that the attributes of the logged in user meet the required + # attributes + # With OIDC, we check userinfo against attribute_requirements + if not self._sso_handler.check_required_attributes( + request, userinfo, self._oidc_attribute_requirements + ): + return # Call the mapper to register/login the user try: From d377b39916e2f913370d1ee766913c91d4e68d14 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 20:57:03 +0200 Subject: [PATCH 02/18] Fix linting issue Signed-off-by: Hubbe King --- synapse/config/oidc_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 6e918787f1db..69a74186d7b7 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -19,8 +19,8 @@ import attr -from synapse.config.sso import SsoAttributeRequirement from synapse.config._util import validate_config +from synapse.config.sso import SsoAttributeRequirement from synapse.python_dependencies import DependencyException, check_requirements from synapse.types import Collection, JsonDict from synapse.util.module_loader import load_module From ff7ad74a4b983f7bfc30fd76d9c3faab788c2147 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 21:37:30 +0200 Subject: [PATCH 03/18] Add test case for oidc attribute_requirements Signed-off-by: Hubbe King --- tests/handlers/test_oidc.py | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 5e9c9c2e88eb..1f0fea79831a 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -989,6 +989,42 @@ def test_null_localpart(self): self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) self.assertRenderedError("mapping_error", "localpart is invalid: ") + @override_config( + { + "oidc_config": { + **DEFAULT_CONFIG, + "attribute_requirements": [ + {"attribute": "test", "value": "foobar"} + ] + } + } + ) + def test_attribute_requirements(self): + """The required attributes must be met from the OIDC userinfo response.""" + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + + # userinfo lacking "test": "foobar" attribute should fail. + userinfo = { + "sub": "tester", + "username": "tester", + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": "foobar" attribute should succeed. + userinfo = { + "sub": "tester", + "username": "tester", + "test": "foobar", + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + + # check that the auth handler got called as expected + auth_handler.complete_sso_login.assert_called_once_with( + "@tester:test", "oidc", ANY, ANY, None, new_user=True + ) + def _generate_oidc_session_token( self, state: str, From a574b718e763d631ce140857f134b5353788eb91 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 21:41:10 +0200 Subject: [PATCH 04/18] Add changelog file Signed-off-by: HubbeKing --- changelog.d/9606.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9606.feature diff --git a/changelog.d/9606.feature b/changelog.d/9606.feature new file mode 100644 index 000000000000..fecf27bf3436 --- /dev/null +++ b/changelog.d/9606.feature @@ -0,0 +1 @@ +Added SsoAttributeRequirements to OpenID Connect-based SSO providers. Contributed by Hubbe King From cc5802f4f72fb61da277666cc3fa00a1be59ec50 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 21:55:45 +0200 Subject: [PATCH 05/18] Fix newsfile Signed-off-by: Hubbe King --- changelog.d/9606.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/9606.feature b/changelog.d/9606.feature index fecf27bf3436..43555166762b 100644 --- a/changelog.d/9606.feature +++ b/changelog.d/9606.feature @@ -1 +1 @@ -Added SsoAttributeRequirements to OpenID Connect-based SSO providers. Contributed by Hubbe King +Added SsoAttributeRequirements to OpenID Connect-based SSO providers. Contributed by Hubbe King. From b2b52ca7cdbf89a6d495c9072eabd3f858c51c6b Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 21:56:26 +0200 Subject: [PATCH 06/18] Fix style and mypy errors Signed-off-by: Hubbe King --- synapse/config/oidc_config.py | 9 ++++++--- synapse/handlers/oidc_handler.py | 5 ++++- tests/handlers/test_oidc.py | 4 +--- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 69a74186d7b7..144e362229ff 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -345,8 +345,8 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): "allow_existing_users": {"type": "boolean"}, "user_mapping_provider": {"type": ["object", "null"]}, "attribute_requirements": { - "type": "array", - "items": SsoAttributeRequirement.JSON_SCHEMA + "type": "array", + "items": SsoAttributeRequirement.JSON_SCHEMA, }, }, } @@ -485,7 +485,10 @@ def _parse_oidc_config_dict( jwt_payload=client_secret_jwt_key_config.get("jwt_payload", {}), ) # parse attribute_requirements from config (list of dicts) into a list of SsoAttributeRequirement - attribute_requirements = [SsoAttributeRequirement(**x) for x in oidc_config.get("attribute_requirements", [])] + attribute_requirements = [ + SsoAttributeRequirement(**x) + for x in oidc_config.get("attribute_requirements", []) + ] return OidcProviderConfig( idp_id=idp_id, diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 1a1f6586c024..23b6aa957200 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -40,6 +40,7 @@ OidcProviderClientSecretJwtKey, OidcProviderConfig, ) +from synapse.config.sso import SsoAttributeRequirement from synapse.handlers.sso import MappingException, UserAttributes from synapse.http.site import SynapseRequest from synapse.logging.context import make_deferred_yieldable @@ -278,7 +279,9 @@ def __init__( self._config = provider self._callback_url = hs.config.oidc_callback_url # type: str - self._oidc_attribute_requirements = provider.attribute_requirements # type: List[SsoAttributeRequirement] + self._oidc_attribute_requirements = ( + provider.attribute_requirements + ) # type: List[SsoAttributeRequirement] self._scopes = provider.scopes self._user_profile_method = provider.user_profile_method diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index 1f0fea79831a..bd9a4b6c552a 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -993,9 +993,7 @@ def test_null_localpart(self): { "oidc_config": { **DEFAULT_CONFIG, - "attribute_requirements": [ - {"attribute": "test", "value": "foobar"} - ] + "attribute_requirements": [{"attribute": "test", "value": "foobar"}], } } ) From b2b4bb4e463d7ad37dcca1929a7b1271e62603d2 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 21:56:47 +0200 Subject: [PATCH 07/18] Regenerate sample config Signed-off-by: Hubbe King --- docs/sample_config.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index c32ee4a897b3..2e92c101eeb8 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1872,6 +1872,19 @@ saml2_config: # When rendering, the Jinja2 templates are given a 'user' variable, # which is set to the claims returned by the UserInfo Endpoint and/or # in the ID Token. +# It is possible to configure Synapse to only allow logins if certain attributes +# match particular values in the OIDC userinfo. The requirements can be listed under +# `attribute_requirements` as shown below. All of the listed attributes must +# match for the login to be permitted. Note that fields listed may need to be added +# to the userinfo return in the OIDC provider, by extending or adding a scope. +# If the attributes listed are not part of the default scopes, these scopes may +# also need to be added to the `scopes` section of the OIDC config. +# +# attribute_requirements: +# - attribute: family_name +# value: "Stephensson" +# - attribute: groups +# value: "admin" # # See https://github.com/matrix-org/synapse/blob/master/docs/openid.md # for information on how to configure these options. @@ -1905,6 +1918,9 @@ oidc_providers: # localpart_template: "{{ user.login }}" # display_name_template: "{{ user.name }}" # email_template: "{{ user.email }}" + # attribute_requirements: + # - attribute: userGroup + # value: "synapseUsers" # For use with Keycloak # @@ -1914,6 +1930,9 @@ oidc_providers: # client_id: "synapse" # client_secret: "copy secret generated in Keycloak UI" # scopes: ["openid", "profile"] + # attribute_requirements: + # - attribute: groups + # value: "admin" # For use with Github # From cd32e1e9e91c086ee17212602f907eced710400d Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 22:00:25 +0200 Subject: [PATCH 08/18] Fix newsfragment file number Signed-off-by: HubbeKing --- changelog.d/{9606.feature => 9609.feature} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{9606.feature => 9609.feature} (100%) diff --git a/changelog.d/9606.feature b/changelog.d/9609.feature similarity index 100% rename from changelog.d/9606.feature rename to changelog.d/9609.feature From aa460d5330cc74ff9dc3366340f933e5977ac28c Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 22:14:16 +0200 Subject: [PATCH 09/18] Fix mypy errors Signed-off-by: Hubbe King --- synapse/config/oidc_config.py | 4 ++-- synapse/handlers/oidc_handler.py | 15 ++++++++++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 144e362229ff..565090534654 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -15,7 +15,7 @@ # limitations under the License. from collections import Counter -from typing import Any, Iterable, List, Mapping, Optional, Tuple, Type +from typing import Iterable, Mapping, Optional, Tuple, Type import attr @@ -600,4 +600,4 @@ class OidcProviderConfig: user_mapping_provider_config = attr.ib() # required attributes to require in userinfo to allow login/registration - attribute_requirements = attr.ib(type=Optional[list]) + attribute_requirements = attr.ib(type=Iterable[SsoAttributeRequirement]) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 23b6aa957200..7bab97a563cb 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -15,7 +15,16 @@ # limitations under the License. import inspect import logging -from typing import TYPE_CHECKING, Dict, Generic, List, Optional, TypeVar, Union +from typing import ( + TYPE_CHECKING, + Dict, + Generic, + Iterable, + List, + Optional, + TypeVar, + Union, +) from urllib.parse import urlencode import attr @@ -280,8 +289,8 @@ def __init__( self._callback_url = hs.config.oidc_callback_url # type: str self._oidc_attribute_requirements = ( - provider.attribute_requirements - ) # type: List[SsoAttributeRequirement] + provider.attribute_requirements + ) # type: Iterable[SsoAttributeRequirement] self._scopes = provider.scopes self._user_profile_method = provider.user_profile_method From 1f5de7392c134f26862ed1855366372aaf95d4e0 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Fri, 12 Mar 2021 22:59:09 +0200 Subject: [PATCH 10/18] Replace debug f-string with %s for py3.5 compatibility Signed-off-by: Hubbe King --- synapse/handlers/oidc_handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 7bab97a563cb..9bd807bec11e 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -867,7 +867,7 @@ async def handle_oidc_callback( ) # otherwise, it's a login - logger.debug(f"Userinfo for OIDC login: {userinfo}") + logger.debug("Userinfo for OIDC login: %s", userinfo) # Ensure that the attributes of the logged in user meet the required # attributes From 1d240eb8d1dc4da8686d9e6941d5e2412d7b6fad Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Mon, 15 Mar 2021 18:11:54 +0200 Subject: [PATCH 11/18] Suggested config changes for clarity Signed-off-by: Hubbe King --- docs/sample_config.yaml | 8 ++++---- synapse/config/oidc_config.py | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 2e92c101eeb8..1e1c96207aff 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1872,13 +1872,13 @@ saml2_config: # When rendering, the Jinja2 templates are given a 'user' variable, # which is set to the claims returned by the UserInfo Endpoint and/or # in the ID Token. +# # It is possible to configure Synapse to only allow logins if certain attributes # match particular values in the OIDC userinfo. The requirements can be listed under # `attribute_requirements` as shown below. All of the listed attributes must -# match for the login to be permitted. Note that fields listed may need to be added -# to the userinfo return in the OIDC provider, by extending or adding a scope. -# If the attributes listed are not part of the default scopes, these scopes may -# also need to be added to the `scopes` section of the OIDC config. +# match for the login to be permitted. Additional attributes can be added to +# userinfo by expanding the `scopes` section of the OIDC config to retrieve +# additional information from the OIDC provider. # # attribute_requirements: # - attribute: family_name diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 565090534654..965d077eda83 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -15,7 +15,7 @@ # limitations under the License. from collections import Counter -from typing import Iterable, Mapping, Optional, Tuple, Type +from typing import Iterable, List, Mapping, Optional, Tuple, Type import attr @@ -191,13 +191,13 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # When rendering, the Jinja2 templates are given a 'user' variable, # which is set to the claims returned by the UserInfo Endpoint and/or # in the ID Token. + # # It is possible to configure Synapse to only allow logins if certain attributes # match particular values in the OIDC userinfo. The requirements can be listed under # `attribute_requirements` as shown below. All of the listed attributes must - # match for the login to be permitted. Note that fields listed may need to be added - # to the userinfo return in the OIDC provider, by extending or adding a scope. - # If the attributes listed are not part of the default scopes, these scopes may - # also need to be added to the `scopes` section of the OIDC config. + # match for the login to be permitted. Additional attributes can be added to + # userinfo by expanding the `scopes` section of the OIDC config to retrieve + # additional information from the OIDC provider. # # attribute_requirements: # - attribute: family_name @@ -600,4 +600,4 @@ class OidcProviderConfig: user_mapping_provider_config = attr.ib() # required attributes to require in userinfo to allow login/registration - attribute_requirements = attr.ib(type=Iterable[SsoAttributeRequirement]) + attribute_requirements = attr.ib(type=List[SsoAttributeRequirement]) From d96e1248d972d76035dd2db6e9b6df9b0be277f2 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Mon, 15 Mar 2021 18:12:45 +0200 Subject: [PATCH 12/18] Suggested changes to oidc_handler comments Signed-off-by: Hubbe King --- synapse/handlers/oidc_handler.py | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 9bd807bec11e..5c04083b13d2 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -15,16 +15,7 @@ # limitations under the License. import inspect import logging -from typing import ( - TYPE_CHECKING, - Dict, - Generic, - Iterable, - List, - Optional, - TypeVar, - Union, -) +from typing import TYPE_CHECKING, Dict, Generic, List, Optional, TypeVar, Union from urllib.parse import urlencode import attr @@ -49,7 +40,6 @@ OidcProviderClientSecretJwtKey, OidcProviderConfig, ) -from synapse.config.sso import SsoAttributeRequirement from synapse.handlers.sso import MappingException, UserAttributes from synapse.http.site import SynapseRequest from synapse.logging.context import make_deferred_yieldable @@ -288,9 +278,7 @@ def __init__( self._config = provider self._callback_url = hs.config.oidc_callback_url # type: str - self._oidc_attribute_requirements = ( - provider.attribute_requirements - ) # type: Iterable[SsoAttributeRequirement] + self._oidc_attribute_requirements = provider.attribute_requirements self._scopes = provider.scopes self._user_profile_method = provider.user_profile_method @@ -870,8 +858,7 @@ async def handle_oidc_callback( logger.debug("Userinfo for OIDC login: %s", userinfo) # Ensure that the attributes of the logged in user meet the required - # attributes - # With OIDC, we check userinfo against attribute_requirements + # attributes by checking the userinfo against attribute_requirements if not self._sso_handler.check_required_attributes( request, userinfo, self._oidc_attribute_requirements ): From d2b7361aed947de68b82f7fef84c4d732313abb2 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Mon, 15 Mar 2021 22:39:07 +0200 Subject: [PATCH 13/18] Wrap non-list userinfo values in lists This ensures that we always match strictly on string values, but match on list contains for list values. Signed-off-by: Hubbe King --- docs/sample_config.yaml | 8 ++++++++ synapse/config/oidc_config.py | 8 ++++++++ synapse/handlers/oidc_handler.py | 6 +++++- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 1e1c96207aff..1c0b1adb23bb 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1880,6 +1880,14 @@ saml2_config: # userinfo by expanding the `scopes` section of the OIDC config to retrieve # additional information from the OIDC provider. # +# An important thing to note here, is that OIDC claims which exist in the userinfo +# as a string will be strictly matched - the attribute matches only if the userinfo +# value exactly matches the configured value in `attribute_requirements`. +# However, if the claim exists in the userinfo as a list, the attribute matches +# if the list contains the value configured in `attribute_requirements`. +# i.e. `family_name` in the userinfo MUST be "Stephensson" in the example below +# i.e. `groups` in the userinfo MUST contain "admin" in the example below +# # attribute_requirements: # - attribute: family_name # value: "Stephensson" diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 965d077eda83..81115df27eb7 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -199,6 +199,14 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # userinfo by expanding the `scopes` section of the OIDC config to retrieve # additional information from the OIDC provider. # + # An important thing to note here, is that OIDC claims which exist in the userinfo + # as a string will be strictly matched - the attribute matches only if the userinfo + # value exactly matches the configured value in `attribute_requirements`. + # However, if the claim exists in the userinfo as a list, the attribute matches + # if the list contains the value configured in `attribute_requirements`. + # i.e. `family_name` in the userinfo MUST be "Stephensson" in the example below + # i.e. `groups` in the userinfo MUST contain "admin" in the example below + # # attribute_requirements: # - attribute: family_name # value: "Stephensson" diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 5c04083b13d2..d962933d386d 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -859,8 +859,12 @@ async def handle_oidc_callback( # Ensure that the attributes of the logged in user meet the required # attributes by checking the userinfo against attribute_requirements + # In order to deal with the fact that OIDC userinfo can contain many + # types of data, we wrap non-list values in lists. if not self._sso_handler.check_required_attributes( - request, userinfo, self._oidc_attribute_requirements + request, + {k: v if isinstance(v, list) else [v] for k, v in userinfo.items()}, + self._oidc_attribute_requirements, ): return From 49add26a37b7b663418adcda836ce19736b5e73f Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Mon, 15 Mar 2021 22:40:26 +0200 Subject: [PATCH 14/18] Add more unit tests for OIDC attribute edge cases This should ensure that things behave as expected. Signed-off-by: Hubbe King --- tests/handlers/test_oidc.py | 98 +++++++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index bd9a4b6c552a..c7796fb837bc 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -1023,6 +1023,104 @@ def test_attribute_requirements(self): "@tester:test", "oidc", ANY, ANY, None, new_user=True ) + @override_config( + { + "oidc_config": { + **DEFAULT_CONFIG, + "attribute_requirements": [{"attribute": "test", "value": "foobar"}], + } + } + ) + def test_attribute_requirements_contains(self): + """Test that auth succeeds if userinfo attribute CONTAINS required value""" + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + # userinfo with "test": ["foobar", "foo", "bar"] attribute should succeed. + userinfo = { + "sub": "tester", + "username": "tester", + "test": ["foobar", "foo", "bar"], + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + + # check that the auth handler got called as expected + auth_handler.complete_sso_login.assert_called_once_with( + "@tester:test", "oidc", ANY, ANY, None, new_user=True + ) + + @override_config( + { + "oidc_config": { + **DEFAULT_CONFIG, + "attribute_requirements": [{"attribute": "test", "value": "foobar"}], + } + } + ) + def test_attribute_requirements_mismatch(self): + """ + Test that auth fails if attributes exist but don't match, + or are non-string values. + """ + auth_handler = self.hs.get_auth_handler() + auth_handler.complete_sso_login = simple_async_mock() + # userinfo with "test": "not_foobar" attribute should fail + userinfo = { + "sub": "tester", + "username": "tester", + "test": "not_foobar", + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": ["foo", "bar"] attribute should fail + userinfo = { + "sub": "tester", + "username": "tester", + "test": ["foo", "bar"], + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": False attribute should fail + # this is largely just to ensure we don't crash here + userinfo = { + "sub": "tester", + "username": "tester", + "test": False, + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": None attribute should fail + # a value of None breaks the OIDC spec, but it's important to not crash here + userinfo = { + "sub": "tester", + "username": "tester", + "test": None, + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": 1 attribute should fail + # this is largely just to ensure we don't crash here + userinfo = { + "sub": "tester", + "username": "tester", + "test": 1, + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + + # userinfo with "test": 3.14 attribute should fail + # this is largely just to ensure we don't crash here + userinfo = { + "sub": "tester", + "username": "tester", + "test": 3.14, + } + self.get_success(_make_callback_with_userinfo(self.hs, userinfo)) + auth_handler.complete_sso_login.assert_not_called() + def _generate_oidc_session_token( self, state: str, From 1a3b60b68afbd9f119e4ac303b49cb5efc18931a Mon Sep 17 00:00:00 2001 From: Hubbe Date: Tue, 16 Mar 2021 08:05:15 +0200 Subject: [PATCH 15/18] Update changelog.d/9609.feature Co-authored-by: Patrick Cloke --- changelog.d/9609.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/9609.feature b/changelog.d/9609.feature index 43555166762b..f3b6342069f6 100644 --- a/changelog.d/9609.feature +++ b/changelog.d/9609.feature @@ -1 +1 @@ -Added SsoAttributeRequirements to OpenID Connect-based SSO providers. Contributed by Hubbe King. +Logins using OpenID Connect can require attributes on the `userinfo` response in order to login. Contributed by Hubbe King. From 55b096bb4b08a8328b52121429204897c8699fe3 Mon Sep 17 00:00:00 2001 From: Hubbe Date: Tue, 16 Mar 2021 16:32:01 +0200 Subject: [PATCH 16/18] Update synapse/config/oidc_config.py Co-authored-by: Patrick Cloke --- synapse/config/oidc_config.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 81115df27eb7..617ca3f0bffb 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -199,13 +199,10 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # userinfo by expanding the `scopes` section of the OIDC config to retrieve # additional information from the OIDC provider. # - # An important thing to note here, is that OIDC claims which exist in the userinfo - # as a string will be strictly matched - the attribute matches only if the userinfo - # value exactly matches the configured value in `attribute_requirements`. - # However, if the claim exists in the userinfo as a list, the attribute matches - # if the list contains the value configured in `attribute_requirements`. - # i.e. `family_name` in the userinfo MUST be "Stephensson" in the example below - # i.e. `groups` in the userinfo MUST contain "admin" in the example below + # If the OIDC claim is a list, then the attribute must match any value in the list. + # Otherwise, it must exactly match the value of the claim. Using the example + # below, the `family_name` claim MUST be "Stephensson", but the `groups` + # claim MUST contain "admin". # # attribute_requirements: # - attribute: family_name From 33dd3e7b486d30ccfae158ecc508f8c23c6c7491 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Tue, 16 Mar 2021 16:32:40 +0200 Subject: [PATCH 17/18] Regenerate sample config Signed-off-by: Hubbe King --- docs/sample_config.yaml | 45 +++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 1c0b1adb23bb..e5a3944ace70 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1738,6 +1738,13 @@ saml2_config: # - attribute: department # value: "sales" + # It is possible to disable creation of new users through SAML logins, meaning + # that only existing matrix users on this homeserver are allowed to login through + # SAML. By setting `allow_new_users` to False, no new users will be generated. + # Defaults to True. + # + # allow_new_users: True + # If the metadata XML contains multiple IdP entities then the `idp_entityid` # option must be set to the entity to redirect users to. # @@ -1834,6 +1841,10 @@ saml2_config: # match a pre-existing account instead of failing. This could be used if # switching from password logins to OIDC. Defaults to false. # +# allow_new_users: set to 'false' to disable new user creation through SSO login +# This means SSO logins are only possible for existing matrix users, through +# the above `allow_existing_users` setting. Default to true. +# # user_mapping_provider: Configuration for how attributes returned from a OIDC # provider are mapped onto a matrix user. This setting has the following # sub-properties: @@ -1873,27 +1884,6 @@ saml2_config: # which is set to the claims returned by the UserInfo Endpoint and/or # in the ID Token. # -# It is possible to configure Synapse to only allow logins if certain attributes -# match particular values in the OIDC userinfo. The requirements can be listed under -# `attribute_requirements` as shown below. All of the listed attributes must -# match for the login to be permitted. Additional attributes can be added to -# userinfo by expanding the `scopes` section of the OIDC config to retrieve -# additional information from the OIDC provider. -# -# An important thing to note here, is that OIDC claims which exist in the userinfo -# as a string will be strictly matched - the attribute matches only if the userinfo -# value exactly matches the configured value in `attribute_requirements`. -# However, if the claim exists in the userinfo as a list, the attribute matches -# if the list contains the value configured in `attribute_requirements`. -# i.e. `family_name` in the userinfo MUST be "Stephensson" in the example below -# i.e. `groups` in the userinfo MUST contain "admin" in the example below -# -# attribute_requirements: -# - attribute: family_name -# value: "Stephensson" -# - attribute: groups -# value: "admin" -# # See https://github.com/matrix-org/synapse/blob/master/docs/openid.md # for information on how to configure these options. # @@ -1926,9 +1916,6 @@ oidc_providers: # localpart_template: "{{ user.login }}" # display_name_template: "{{ user.name }}" # email_template: "{{ user.email }}" - # attribute_requirements: - # - attribute: userGroup - # value: "synapseUsers" # For use with Keycloak # @@ -1938,9 +1925,6 @@ oidc_providers: # client_id: "synapse" # client_secret: "copy secret generated in Keycloak UI" # scopes: ["openid", "profile"] - # attribute_requirements: - # - attribute: groups - # value: "admin" # For use with Github # @@ -1990,6 +1974,13 @@ cas_config: # userGroup: "staff" # department: None + # It is possible to disable creation of new users through CAS login, meaning + # that only existing matrix users on this homeserver are allowed to login through + # CAS. By setting `allow_new_users` to False, no new users will be generated. + # Defaults to True. + # + # allow_new_users: True + # Additional settings to use with single-sign on systems such as OpenID Connect, # SAML2 and CAS. From 4989d8cb271ebfd89457d04b0e7ef269c098a4b3 Mon Sep 17 00:00:00 2001 From: HubbeKing Date: Tue, 16 Mar 2021 16:41:26 +0200 Subject: [PATCH 18/18] Actually regenerate sample config, properly this time My dev system is a mess. Signed-off-by: Hubbe King --- docs/sample_config.yaml | 42 +++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index e5a3944ace70..331e70fb29e8 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1738,13 +1738,6 @@ saml2_config: # - attribute: department # value: "sales" - # It is possible to disable creation of new users through SAML logins, meaning - # that only existing matrix users on this homeserver are allowed to login through - # SAML. By setting `allow_new_users` to False, no new users will be generated. - # Defaults to True. - # - # allow_new_users: True - # If the metadata XML contains multiple IdP entities then the `idp_entityid` # option must be set to the entity to redirect users to. # @@ -1841,10 +1834,6 @@ saml2_config: # match a pre-existing account instead of failing. This could be used if # switching from password logins to OIDC. Defaults to false. # -# allow_new_users: set to 'false' to disable new user creation through SSO login -# This means SSO logins are only possible for existing matrix users, through -# the above `allow_existing_users` setting. Default to true. -# # user_mapping_provider: Configuration for how attributes returned from a OIDC # provider are mapped onto a matrix user. This setting has the following # sub-properties: @@ -1884,6 +1873,24 @@ saml2_config: # which is set to the claims returned by the UserInfo Endpoint and/or # in the ID Token. # +# It is possible to configure Synapse to only allow logins if certain attributes +# match particular values in the OIDC userinfo. The requirements can be listed under +# `attribute_requirements` as shown below. All of the listed attributes must +# match for the login to be permitted. Additional attributes can be added to +# userinfo by expanding the `scopes` section of the OIDC config to retrieve +# additional information from the OIDC provider. +# +# If the OIDC claim is a list, then the attribute must match any value in the list. +# Otherwise, it must exactly match the value of the claim. Using the example +# below, the `family_name` claim MUST be "Stephensson", but the `groups` +# claim MUST contain "admin". +# +# attribute_requirements: +# - attribute: family_name +# value: "Stephensson" +# - attribute: groups +# value: "admin" +# # See https://github.com/matrix-org/synapse/blob/master/docs/openid.md # for information on how to configure these options. # @@ -1916,6 +1923,9 @@ oidc_providers: # localpart_template: "{{ user.login }}" # display_name_template: "{{ user.name }}" # email_template: "{{ user.email }}" + # attribute_requirements: + # - attribute: userGroup + # value: "synapseUsers" # For use with Keycloak # @@ -1925,6 +1935,9 @@ oidc_providers: # client_id: "synapse" # client_secret: "copy secret generated in Keycloak UI" # scopes: ["openid", "profile"] + # attribute_requirements: + # - attribute: groups + # value: "admin" # For use with Github # @@ -1974,13 +1987,6 @@ cas_config: # userGroup: "staff" # department: None - # It is possible to disable creation of new users through CAS login, meaning - # that only existing matrix users on this homeserver are allowed to login through - # CAS. By setting `allow_new_users` to False, no new users will be generated. - # Defaults to True. - # - # allow_new_users: True - # Additional settings to use with single-sign on systems such as OpenID Connect, # SAML2 and CAS.