From e8d43bc9f98ae23f4c531fb81899298334667434 Mon Sep 17 00:00:00 2001 From: Benjamin Koch Date: Thu, 4 Jun 2020 00:57:23 +0200 Subject: [PATCH 1/7] add config option for always using userinfo endpoint --- synapse/config/oidc_config.py | 6 ++++++ synapse/handlers/oidc_handler.py | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 586038078f86..f3ef1cf8bc8e 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -51,6 +51,7 @@ def read_config(self, config, **kwargs): "client_auth_method", "client_secret_basic" ) self.oidc_scopes = oidc_config.get("scopes", ["openid"]) + self.oidc_uses_userinfo = oidc_config.get("uses_userinfo", False) self.oidc_authorization_endpoint = oidc_config.get("authorization_endpoint") self.oidc_token_endpoint = oidc_config.get("token_endpoint") self.oidc_userinfo_endpoint = oidc_config.get("userinfo_endpoint") @@ -118,6 +119,11 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # #scopes: ["openid"] + # always use userinfo endpoint. This is required for providers that don't include user + # information in the token response, e.g. Gitlab. + # + #uses_userinfo: false + # the oauth2 authorization endpoint. Required if provider discovery is disabled. # #authorization_endpoint: "https://accounts.example.com/oauth2/auth" diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 4ba8c7fda502..6bc336fd2c2c 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -93,6 +93,7 @@ class OidcHandler: def __init__(self, hs: HomeServer): self._callback_url = hs.config.oidc_callback_url # type: str self._scopes = hs.config.oidc_scopes # type: List[str] + self._uses_userinfo_config = hs.config.oidc_uses_userinfo # type: bool self._client_auth = ClientAuth( hs.config.oidc_client_id, hs.config.oidc_client_secret, @@ -224,8 +225,7 @@ def _uses_userinfo(self) -> bool: ``access_token`` with the ``userinfo_endpoint``. """ - # Maybe that should be user-configurable and not inferred? - return "openid" not in self._scopes + return self._uses_userinfo_config or "openid" not in self._scopes async def load_metadata(self) -> OpenIDProviderMetadata: """Load and validate the provider metadata. From a1821f320f7a42682a71b79eede1c03879c26c44 Mon Sep 17 00:00:00 2001 From: Benjamin Koch Date: Thu, 4 Jun 2020 01:43:36 +0200 Subject: [PATCH 2/7] add config option that allows merging OpenID Connect users with existing users --- synapse/config/oidc_config.py | 5 +++++ synapse/handlers/oidc_handler.py | 22 +++++++++++++--------- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index f3ef1cf8bc8e..f1a3f9d1df5a 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -58,6 +58,7 @@ def read_config(self, config, **kwargs): self.oidc_jwks_uri = oidc_config.get("jwks_uri") self.oidc_subject_claim = oidc_config.get("subject_claim", "sub") self.oidc_skip_verification = oidc_config.get("skip_verification", False) + self.oidc_merge_with_existing_users = oidc_config.get("merge_with_existing_users", False) ump_config = oidc_config.get("user_mapping_provider", {}) ump_config.setdefault("module", DEFAULT_USER_MAPPING_PROVIDER) @@ -146,6 +147,10 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # #skip_verification: false + # if user already exists, add oidc token to that account instead of failing. Defaults to false. + # + #merge_with_existing_users: false + # An external module can be provided here as a custom solution to mapping # attributes returned from a OIDC provider onto a matrix user. diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 6bc336fd2c2c..7d475ded2ce0 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -113,6 +113,7 @@ def __init__(self, hs: HomeServer): hs.config.oidc_user_mapping_provider_config ) # type: OidcMappingProvider self._skip_verification = hs.config.oidc_skip_verification # type: bool + self._merge_with_existing_users = hs.config.oidc_merge_with_existing_users # type: bool self._http_client = hs.get_proxied_http_client() self._auth_handler = hs.get_auth_handler() @@ -884,17 +885,20 @@ async def _map_userinfo_to_user(self, userinfo: UserInfo, token: Token) -> str: user_id = UserID(localpart, self._hostname) if await self._datastore.get_users_by_id_case_insensitive(user_id.to_string()): - # This mxid is taken - raise MappingException( - "mxid '{}' is already taken".format(user_id.to_string()) + if self._merge_with_existing_users: + registered_user_id = user_id.to_string() + else: + # This mxid is taken + raise MappingException( + "mxid '{}' is already taken".format(user_id.to_string()) + ) + else: + # It's the first time this user is logging in and the mapped mxid was + # not taken, register the user + registered_user_id = await self._registration_handler.register_user( + localpart=localpart, default_display_name=attributes["display_name"], ) - # It's the first time this user is logging in and the mapped mxid was - # not taken, register the user - registered_user_id = await self._registration_handler.register_user( - localpart=localpart, default_display_name=attributes["display_name"], - ) - await self._datastore.record_user_external_id( self._auth_provider_id, remote_user_id, registered_user_id, ) From 073b3b65a91e1f3bf86138a1822ee0d7182c8926 Mon Sep 17 00:00:00 2001 From: Tdxdxoz Date: Wed, 26 Aug 2020 23:01:39 +0800 Subject: [PATCH 3/7] some format detail --- synapse/config/oidc_config.py | 3 +-- synapse/handlers/oidc_handler.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 94f3d82b8f81..22033056f2d0 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -169,7 +169,6 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # #merge_with_existing_users: false - # An external module can be provided here as a custom solution to mapping # attributes returned from a OIDC provider onto a matrix user. # @@ -185,7 +184,7 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # Custom configuration values for the module. This section will be passed as # a Python dictionary to the user mapping provider module's `parse_config` # method. - + # # The examples below are intended for the default provider: they should be # changed if using a custom provider. # diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 2b97e654a3d8..fd69ab1db85d 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -917,7 +917,7 @@ async def _map_userinfo_to_user( # not taken, register the user registered_user_id = await self._registration_handler.register_user( localpart=localpart, default_display_name=attributes["display_name"], - user_agent_ips=(user_agent, ip_address), + user_agent_ips=(user_agent, ip_address), ) await self._datastore.record_user_external_id( self._auth_provider_id, remote_user_id, registered_user_id, From 94e52a1346779902a68729db4dd12984fda27618 Mon Sep 17 00:00:00 2001 From: Tdxdxoz Date: Wed, 26 Aug 2020 23:13:59 +0800 Subject: [PATCH 4/7] fix checks --- changelog.d/8180.feature | 1 + docs/sample_config.yaml | 9 +++++++++ synapse/config/oidc_config.py | 6 ++++-- synapse/handlers/oidc_handler.py | 7 +++++-- 4 files changed, 19 insertions(+), 4 deletions(-) create mode 100644 changelog.d/8180.feature diff --git a/changelog.d/8180.feature b/changelog.d/8180.feature new file mode 100644 index 000000000000..9ce9129de144 --- /dev/null +++ b/changelog.d/8180.feature @@ -0,0 +1 @@ +This adds config option for always using userinfo endpoint and adds config option that allows merging OpenID Connect users with existing users. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 3528d9e11f5c..7ad5720b287f 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1671,6 +1671,11 @@ oidc_config: # #authorization_endpoint: "https://accounts.example.com/oauth2/auth" + # always use userinfo endpoint. This is required for providers that don't include user + # information in the token response, e.g. Gitlab. + # + #uses_userinfo: false + # the oauth2 token endpoint. Required if provider discovery is disabled. # #token_endpoint: "https://accounts.example.com/oauth2/token" @@ -1693,6 +1698,10 @@ oidc_config: # #skip_verification: true + # if user already exists, add oidc token to that account instead of failing. Defaults to false. + # + #merge_with_existing_users: false + # An external module can be provided here as a custom solution to mapping # attributes returned from a OIDC provider onto a matrix user. # diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 22033056f2d0..6d1ebc8e5ff9 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -57,7 +57,9 @@ def read_config(self, config, **kwargs): self.oidc_userinfo_endpoint = oidc_config.get("userinfo_endpoint") self.oidc_jwks_uri = oidc_config.get("jwks_uri") self.oidc_skip_verification = oidc_config.get("skip_verification", False) - self.oidc_merge_with_existing_users = oidc_config.get("merge_with_existing_users", False) + self.oidc_merge_with_existing_users = oidc_config.get( + "merge_with_existing_users", False + ) ump_config = oidc_config.get("user_mapping_provider", {}) ump_config.setdefault("module", DEFAULT_USER_MAPPING_PROVIDER) @@ -137,7 +139,7 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # the oauth2 authorization endpoint. Required if provider discovery is disabled. # #authorization_endpoint: "https://accounts.example.com/oauth2/auth" - + # always use userinfo endpoint. This is required for providers that don't include user # information in the token response, e.g. Gitlab. # diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index fd69ab1db85d..a00e4b848a52 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -115,7 +115,9 @@ def __init__(self, hs: "HomeServer"): hs.config.oidc_user_mapping_provider_config ) # type: OidcMappingProvider self._skip_verification = hs.config.oidc_skip_verification # type: bool - self._merge_with_existing_users = hs.config.oidc_merge_with_existing_users # type: bool + self._merge_with_existing_users = ( + hs.config.oidc_merge_with_existing_users + ) # type: bool self._http_client = hs.get_proxied_http_client() self._auth_handler = hs.get_auth_handler() @@ -916,7 +918,8 @@ async def _map_userinfo_to_user( # It's the first time this user is logging in and the mapped mxid was # not taken, register the user registered_user_id = await self._registration_handler.register_user( - localpart=localpart, default_display_name=attributes["display_name"], + localpart=localpart, + default_display_name=attributes["display_name"], user_agent_ips=(user_agent, ip_address), ) await self._datastore.record_user_external_id( From 5c0ba94f7d523d1a2cc6cde4d4d88e5a6793c88e Mon Sep 17 00:00:00 2001 From: Tdxdxoz Date: Wed, 9 Sep 2020 21:17:01 +0800 Subject: [PATCH 5/7] only for for the existing users --- changelog.d/8180.feature | 2 +- docs/sample_config.yaml | 5 ----- synapse/config/oidc_config.py | 6 ------ synapse/handlers/oidc_handler.py | 4 ++-- 4 files changed, 3 insertions(+), 14 deletions(-) diff --git a/changelog.d/8180.feature b/changelog.d/8180.feature index 9ce9129de144..2d01f670392d 100644 --- a/changelog.d/8180.feature +++ b/changelog.d/8180.feature @@ -1 +1 @@ -This adds config option for always using userinfo endpoint and adds config option that allows merging OpenID Connect users with existing users. +This adds config option that allows merging OpenID Connect users with existing users. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 7ad5720b287f..24d802cd6b42 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1671,11 +1671,6 @@ oidc_config: # #authorization_endpoint: "https://accounts.example.com/oauth2/auth" - # always use userinfo endpoint. This is required for providers that don't include user - # information in the token response, e.g. Gitlab. - # - #uses_userinfo: false - # the oauth2 token endpoint. Required if provider discovery is disabled. # #token_endpoint: "https://accounts.example.com/oauth2/token" diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index 6d1ebc8e5ff9..cd1e392db36b 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -51,7 +51,6 @@ def read_config(self, config, **kwargs): "client_auth_method", "client_secret_basic" ) self.oidc_scopes = oidc_config.get("scopes", ["openid"]) - self.oidc_uses_userinfo = oidc_config.get("uses_userinfo", False) self.oidc_authorization_endpoint = oidc_config.get("authorization_endpoint") self.oidc_token_endpoint = oidc_config.get("token_endpoint") self.oidc_userinfo_endpoint = oidc_config.get("userinfo_endpoint") @@ -140,11 +139,6 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # #authorization_endpoint: "https://accounts.example.com/oauth2/auth" - # always use userinfo endpoint. This is required for providers that don't include user - # information in the token response, e.g. Gitlab. - # - #uses_userinfo: false - # the oauth2 token endpoint. Required if provider discovery is disabled. # #token_endpoint: "https://accounts.example.com/oauth2/token" diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index a00e4b848a52..d10596cb3ffb 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -96,7 +96,6 @@ def __init__(self, hs: "HomeServer"): self.hs = hs self._callback_url = hs.config.oidc_callback_url # type: str self._scopes = hs.config.oidc_scopes # type: List[str] - self._uses_userinfo_config = hs.config.oidc_uses_userinfo # type: bool self._client_auth = ClientAuth( hs.config.oidc_client_id, hs.config.oidc_client_secret, @@ -223,7 +222,8 @@ def _uses_userinfo(self) -> bool: ``access_token`` with the ``userinfo_endpoint``. """ - return self._uses_userinfo_config or "openid" not in self._scopes + # Maybe that should be user-configurable and not inferred? + return "openid" not in self._scopes async def load_metadata(self) -> OpenIDProviderMetadata: """Load and validate the provider metadata. From 11b8138465efb8862e09880bae6d10dc81cf8929 Mon Sep 17 00:00:00 2001 From: Tdxdxoz Date: Fri, 11 Sep 2020 18:14:18 +0800 Subject: [PATCH 6/7] fix bug and variable name --- changelog.d/8180.feature | 2 +- docs/sample_config.yaml | 4 ++-- synapse/config/oidc_config.py | 8 ++++---- synapse/handlers/oidc_handler.py | 14 ++++++++------ 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/changelog.d/8180.feature b/changelog.d/8180.feature index 2d01f670392d..4ee5b6a56e37 100644 --- a/changelog.d/8180.feature +++ b/changelog.d/8180.feature @@ -1 +1 @@ -This adds config option that allows merging OpenID Connect users with existing users. +Add a configuration option that allows existing users to log in with OpenID Connect. Contributed by @BBBSnowball and @OmmyZhang. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 24d802cd6b42..705e45aaaab5 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1693,9 +1693,9 @@ oidc_config: # #skip_verification: true - # if user already exists, add oidc token to that account instead of failing. Defaults to false. + # Uncomment to add oidc token to the account instead of failing when user already exists. Defaults to false. # - #merge_with_existing_users: false + #allow_existing_users: true # An external module can be provided here as a custom solution to mapping # attributes returned from a OIDC provider onto a matrix user. diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index cd1e392db36b..dc8da24de7ca 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -56,8 +56,8 @@ def read_config(self, config, **kwargs): self.oidc_userinfo_endpoint = oidc_config.get("userinfo_endpoint") self.oidc_jwks_uri = oidc_config.get("jwks_uri") self.oidc_skip_verification = oidc_config.get("skip_verification", False) - self.oidc_merge_with_existing_users = oidc_config.get( - "merge_with_existing_users", False + self.oidc_allow_existing_users = oidc_config.get( + "allow_existing_users", False ) ump_config = oidc_config.get("user_mapping_provider", {}) @@ -161,9 +161,9 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # #skip_verification: true - # if user already exists, add oidc token to that account instead of failing. Defaults to false. + # Uncomment to add oidc token to the account instead of failing when user already exists. Defaults to false. # - #merge_with_existing_users: false + #allow_existing_users: true # An external module can be provided here as a custom solution to mapping # attributes returned from a OIDC provider onto a matrix user. diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index d10596cb3ffb..33fec817ed83 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -114,8 +114,8 @@ def __init__(self, hs: "HomeServer"): hs.config.oidc_user_mapping_provider_config ) # type: OidcMappingProvider self._skip_verification = hs.config.oidc_skip_verification # type: bool - self._merge_with_existing_users = ( - hs.config.oidc_merge_with_existing_users + self._allow_existing_users = ( + hs.config.oidc_allow_existing_users ) # type: bool self._http_client = hs.get_proxied_http_client() @@ -852,7 +852,8 @@ async def _map_userinfo_to_user( If we don't find the user that way, we should register the user, mapping the localpart and the display name from the UserInfo. - If a user already exists with the mxid we've mapped, raise an exception. + If a user already exists with the mxid we've mapped and allow_existing_users + is disabled , raise an exception. Args: userinfo: an object representing the user @@ -906,9 +907,10 @@ async def _map_userinfo_to_user( localpart = map_username_to_mxid_localpart(attributes["localpart"]) user_id = UserID(localpart, self._hostname) - if await self._datastore.get_users_by_id_case_insensitive(user_id.to_string()): - if self._merge_with_existing_users: - registered_user_id = user_id.to_string() + matches = await self._datastore.get_users_by_id_case_insensitive(user_id.to_string()) + if matches: + if self._allow_existing_users: + registered_user_id = next(iter(matches)) else: # This mxid is taken raise MappingException( From d28b476bffcc76742787efce8ba4291b38235d28 Mon Sep 17 00:00:00 2001 From: Tdxdxoz Date: Fri, 11 Sep 2020 18:23:51 +0800 Subject: [PATCH 7/7] fix code style --- synapse/config/oidc_config.py | 4 +--- synapse/handlers/oidc_handler.py | 8 ++++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/synapse/config/oidc_config.py b/synapse/config/oidc_config.py index dc8da24de7ca..c9561771c892 100644 --- a/synapse/config/oidc_config.py +++ b/synapse/config/oidc_config.py @@ -56,9 +56,7 @@ def read_config(self, config, **kwargs): self.oidc_userinfo_endpoint = oidc_config.get("userinfo_endpoint") self.oidc_jwks_uri = oidc_config.get("jwks_uri") self.oidc_skip_verification = oidc_config.get("skip_verification", False) - self.oidc_allow_existing_users = oidc_config.get( - "allow_existing_users", False - ) + self.oidc_allow_existing_users = oidc_config.get("allow_existing_users", False) ump_config = oidc_config.get("user_mapping_provider", {}) ump_config.setdefault("module", DEFAULT_USER_MAPPING_PROVIDER) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 33fec817ed83..513a333e4678 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -114,9 +114,7 @@ def __init__(self, hs: "HomeServer"): hs.config.oidc_user_mapping_provider_config ) # type: OidcMappingProvider self._skip_verification = hs.config.oidc_skip_verification # type: bool - self._allow_existing_users = ( - hs.config.oidc_allow_existing_users - ) # type: bool + self._allow_existing_users = hs.config.oidc_allow_existing_users # type: bool self._http_client = hs.get_proxied_http_client() self._auth_handler = hs.get_auth_handler() @@ -907,7 +905,9 @@ async def _map_userinfo_to_user( localpart = map_username_to_mxid_localpart(attributes["localpart"]) user_id = UserID(localpart, self._hostname) - matches = await self._datastore.get_users_by_id_case_insensitive(user_id.to_string()) + matches = await self._datastore.get_users_by_id_case_insensitive( + user_id.to_string() + ) if matches: if self._allow_existing_users: registered_user_id = next(iter(matches))