Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Allow OIDC for existing users #8180

Closed
wants to merge 10 commits into from
1 change: 1 addition & 0 deletions changelog.d/8180.feature
Original file line number Diff line number Diff line change
@@ -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.
9 changes: 9 additions & 0 deletions docs/sample_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
#
Expand Down
13 changes: 13 additions & 0 deletions synapse/config/oidc_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,15 @@ 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")
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
)

ump_config = oidc_config.get("user_mapping_provider", {})
ump_config.setdefault("module", DEFAULT_USER_MAPPING_PROVIDER)
Expand Down Expand Up @@ -136,6 +140,11 @@ 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"
Expand All @@ -158,6 +167,10 @@ 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.
#
#merge_with_existing_users: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem a great name for this option. How about allow_existing_users ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/matrix-org/synapse/blob/develop/docs/code_style.md#configuration-file-format says:

For boolean (on/off) options, convention is that this example should be the opposite to the default (so the comment will end with "Uncomment the following to enable [or disable] ."

please could you follow the conventions.


# An external module can be provided here as a custom solution to mapping
# attributes returned from a OIDC provider onto a matrix user.
#
Expand Down
33 changes: 19 additions & 14 deletions synapse/handlers/oidc_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ 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,
Expand All @@ -114,6 +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._http_client = hs.get_proxied_http_client()
self._auth_handler = hs.get_auth_handler()
Expand Down Expand Up @@ -219,8 +223,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.
Expand Down Expand Up @@ -904,19 +907,21 @@ async def _map_userinfo_to_user(

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not a given that the registered user id is the same as the id we looked up (note: "_case_insensitive".) get_users_by_id_case_insensitive returns a dict of potiental matches. please use it.

else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the docstring on this method says:

    If a user already exists with the mxid we've mapped, raise an exception.

Please update it.

# 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"],
user_agent_ips=(user_agent, ip_address),
)

# 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"],
user_agent_ips=(user_agent, ip_address),
)

await self._datastore.record_user_external_id(
self._auth_provider_id, remote_user_id, registered_user_id,
)
Expand Down