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
Closed

Allow OIDC for existing users #8180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e8d43bc
add config option for always using userinfo endpoint
BBBSnowball Jun 3, 2020
a1821f3
add config option that allows merging OpenID Connect users with exist…
BBBSnowball Jun 3, 2020
0ab81d3
Merge branch 'develop' into feature-allows-merging-oidc-with-existing…
OmmyZhang Aug 26, 2020
073b3b6
some format detail
OmmyZhang Aug 26, 2020
94e52a1
fix checks
OmmyZhang Aug 26, 2020
74e6ddd
Merge branch 'develop' into feature-allows-merging-oidc-with-existing…
OmmyZhang Aug 27, 2020
5c0ba94
only for for the existing users
OmmyZhang Sep 9, 2020
11b8138
fix bug and variable name
OmmyZhang Sep 11, 2020
d28b476
fix code style
OmmyZhang Sep 11, 2020
2b4d7b0
Merge remote-tracking branch 'origin/develop' into feature-allows-mer…
OmmyZhang Sep 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8180.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a configuration option that allows existing users to log in with OpenID Connect. Contributed by @BBBSnowball and @OmmyZhang.
4 changes: 4 additions & 0 deletions docs/sample_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1711,6 +1711,10 @@ oidc_config:
#
#skip_verification: true

# Uncomment to add oidc token to the account instead of failing when user already exists. Defaults to 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.
#
Expand Down
5 changes: 5 additions & 0 deletions synapse/config/oidc_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +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)

ump_config = oidc_config.get("user_mapping_provider", {})
ump_config.setdefault("module", DEFAULT_USER_MAPPING_PROVIDER)
Expand Down Expand Up @@ -158,6 +159,10 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs):
#
#skip_verification: true

# Uncomment to add oidc token to the account instead of failing when user already exists. Defaults to false.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this comment is very descriptive -- what's the token that's being added? Why would I turn this on?

Suggested change
# Uncomment to add oidc token to the account instead of failing when user already exists. Defaults to false.
# Uncomment to allow a user logging in via OIDC to match a pre-existing account instead
# of failing. This could be used if switching from password logins to OIDC. Defaults to 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.
#
Expand Down
35 changes: 21 additions & 14 deletions synapse/handlers/oidc_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +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._http_client = hs.get_proxied_http_client()
self._auth_handler = hs.get_auth_handler()
Expand Down Expand Up @@ -849,7 +850,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.
Comment on lines +853 to +854
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If a user already exists with the mxid we've mapped and allow_existing_users
is disabled , 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
Expand Down Expand Up @@ -906,20 +908,25 @@ 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()):
# This mxid is taken
raise MappingException(
"mxid '{}' is already taken".format(user_id.to_string())
)

# 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),
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))
Comment on lines +915 to +916
Copy link
Member

Choose a reason for hiding this comment

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

Can matches have multiple items? Should we still error in that case? It feels arbitrary to choose the first one. At the very least I think we should log something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there can't be more than one item as the same (case insensitive) usernames are not allowed. This is also why we need get_users_by_id_case_insensitive.

synapse/synapse/handlers/register.py

Lines 111 to 116 in efb6b66

users = await self.store.get_users_by_id_case_insensitive(user_id)
if users:
if not guest_access_token:
raise SynapseError(
400, "User ID already taken.", errcode=Codes.USER_IN_USE
)

Maybe we should log something if there're more than one, to show that something goes wrong? But it's a little strange. If we do, we should also do this in many other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I find auth considers more. But I still can't understand why there can be multiple matches. Maybe for backward compatibility？

synapse/synapse/handlers/auth.py

Lines 753 to 758 in efb6b66

elif user_id in user_infos:
# multiple matches, but one is exact
result = (user_id, user_infos[user_id])
else:
# multiple matches, none of them exact
logger.warning(

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"],
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