Allow OIDC for existing users #8180

OmmyZhang · 2020-08-26T15:05:24Z

Allows merging oidc with existing users and allows always using userinfo endpoint.
a patch by @BBBSnowball

fix #7633 and #8155

…ing users

…-users

clokep · 2020-08-26T17:30:35Z

@OmmyZhang Can you merge develop in again? We had a broken test that got fixed an hour or so ago. Sorry about that! 👍

clokep · 2020-08-26T17:31:09Z

This replaces #8178 (just to cross-link).

…-users

richvdh

please could you separate this into two PRs: one for the gitlab fix, and the other for the existing users?

OmmyZhang · 2020-09-09T09:58:32Z

@richvdh Sure!
This PR is based on BBBSnowball's patch, so maybe it's not good to rewrite all changes (and erase his contribution). How about this: I will undo changes for the gitlab fix, and after this PR is merged, I will revert this undoing (on another branch) to make the other PR.

clokep · 2020-09-09T11:13:01Z

@richvdh Sure!
This PR is based on BBBSnowball's patch, so maybe it's not good to rewrite all changes (and erase his contribution). How about this: I will undo changes for the gitlab fix, and after this PR is merged, I will revert this undoing (on another branch) to make the other PR.

I think the changes for GitLab are already up as #7658.

OmmyZhang · 2020-09-09T11:33:53Z

@richvdh Sure!
This PR is based on BBBSnowball's patch, so maybe it's not good to rewrite all changes (and erase his contribution). How about this: I will undo changes for the gitlab fix, and after this PR is merged, I will revert this undoing (on another branch) to make the other PR.

I think the changes for GitLab are already up as #7658.

Great. Now things are easier.

richvdh

thanks for this! A few comments.

richvdh · 2020-09-10T18:55:17Z

synapse/handlers/oidc_handler.py

-            raise MappingException(
-                "mxid '{}' is already taken".format(user_id.to_string())
+            if self._merge_with_existing_users:
+                registered_user_id = user_id.to_string()


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.

richvdh · 2020-09-10T18:56:13Z

synapse/handlers/oidc_handler.py

+            if self._merge_with_existing_users:
+                registered_user_id = user_id.to_string()
+            else:


the docstring on this method says:

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

Please update it.

richvdh · 2020-09-10T18:57:15Z

changelog.d/8180.feature

@@ -0,0 +1 @@
+This adds config option that allows merging OpenID Connect users with existing users. 


Suggested change

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.

richvdh · 2020-09-10T18:58:08Z

synapse/config/oidc_config.py

@@ -158,6 +161,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


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

richvdh · 2020-09-10T18:59:28Z

synapse/config/oidc_config.py

+          # if user already exists, add oidc token to that account instead of failing. Defaults to false.
+          #
+          #merge_with_existing_users: false


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.

…ging-oidc-with-existing-users

OmmyZhang · 2020-09-12T08:12:59Z

@richvdh thanks for your suggestions. Now it's updated.

clokep

I think it is close! Just a couple more details to work out!

clokep · 2020-09-17T19:03:44Z

synapse/handlers/oidc_handler.py

+            if self._allow_existing_users:
+                registered_user_id = next(iter(matches))


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.

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.

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(

clokep · 2020-09-17T19:06:18Z

synapse/handlers/oidc_handler.py

+        If a user already exists with the mxid we've mapped and allow_existing_users
+        is disabled , raise an exception.


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.

clokep · 2020-09-17T19:10:59Z

synapse/config/oidc_config.py

@@ -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.


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.

OmmyZhang · 2020-09-18T06:07:18Z

Sorry I have to create a new PR as I realized that this branch can't be auto-updated after I delete my repository and re-fork from matrix-org/synapse.

#8345 will replace this one.

BBBSnowball and others added 5 commits June 4, 2020 00:57

add config option for always using userinfo endpoint

e8d43bc

add config option that allows merging OpenID Connect users with exist…

a1821f3

…ing users

Merge branch 'develop' into feature-allows-merging-oidc-with-existing…

0ab81d3

…-users

some format detail

073b3b6

fix checks

94e52a1

Merge branch 'develop' into feature-allows-merging-oidc-with-existing…

74e6ddd

…-users

OmmyZhang mentioned this pull request Aug 27, 2020

Use OpenID Connect users with existing accounts #8178

Closed

clokep requested a review from a team September 4, 2020 11:30

richvdh changed the title ~~Feature allows merging oidc with existing users~~ Allow OIDC for existing users Sep 4, 2020

richvdh suggested changes Sep 4, 2020

View reviewed changes

clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Sep 4, 2020

only for for the existing users

5c0ba94

OmmyZhang requested a review from richvdh September 10, 2020 08:29

richvdh removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Sep 10, 2020

richvdh suggested changes Sep 10, 2020

View reviewed changes

richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Sep 10, 2020

OmmyZhang added 3 commits September 11, 2020 18:14

fix bug and variable name

11b8138

fix code style

d28b476

Merge remote-tracking branch 'origin/develop' into feature-allows-mer…

2b4d7b0

…ging-oidc-with-existing-users

clokep requested a review from a team September 14, 2020 11:28

clokep suggested changes Sep 17, 2020

View reviewed changes

OmmyZhang mentioned this pull request Sep 18, 2020

Allow OIDC for existing users #8345

Merged

OmmyZhang closed this Sep 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow OIDC for existing users #8180

Allow OIDC for existing users #8180

OmmyZhang commented Aug 26, 2020

clokep commented Aug 26, 2020

clokep commented Aug 26, 2020

richvdh left a comment

OmmyZhang commented Sep 9, 2020

clokep commented Sep 9, 2020

OmmyZhang commented Sep 9, 2020

richvdh left a comment

richvdh Sep 10, 2020

richvdh Sep 10, 2020

richvdh Sep 10, 2020

richvdh Sep 10, 2020

richvdh Sep 10, 2020

OmmyZhang commented Sep 12, 2020

clokep left a comment

clokep Sep 17, 2020

OmmyZhang Sep 18, 2020

OmmyZhang Sep 18, 2020

clokep Sep 17, 2020

clokep Sep 17, 2020

OmmyZhang commented Sep 18, 2020

		@@ -0,0 +1 @@
		This 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.
	Add a configuration option that allows existing users to log in with OpenID Connect. Contributed by @BBBSnowball and @OmmyZhang.

		if self._allow_existing_users:
		registered_user_id = next(iter(matches))

	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
	)

	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(

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

	# 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 OIDC for existing users #8180

Allow OIDC for existing users #8180

Conversation

OmmyZhang commented Aug 26, 2020

clokep commented Aug 26, 2020

clokep commented Aug 26, 2020

richvdh left a comment

Choose a reason for hiding this comment

OmmyZhang commented Sep 9, 2020

clokep commented Sep 9, 2020

OmmyZhang commented Sep 9, 2020

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmmyZhang commented Sep 12, 2020

clokep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmmyZhang commented Sep 18, 2020