Allow OIDC for existing users by OmmyZhang · Pull Request #8180 · matrix-org/synapse

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


+          # 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

          #
          #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

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

Uh oh!

Conversation

OmmyZhang commented Aug 26, 2020

Uh oh!

clokep commented Aug 26, 2020

Uh oh!

clokep commented Aug 26, 2020

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

OmmyZhang commented Sep 9, 2020

Uh oh!

clokep commented Sep 9, 2020

Uh oh!

OmmyZhang commented Sep 9, 2020

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmmyZhang commented Sep 12, 2020

Uh oh!

clokep left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmmyZhang commented Sep 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants