Use OpenID Connect users with existing accounts

[BBBSnowball]

I have a homeserver that is using LDAP and I want to migrate to OpenID Connect. This requires that existing users can login with OpenID Connect. If an existing user tries to login, I get the following error and login doesn't work: synapse.handlers.oidc_handler.MappingException: mxid '@snowball:test.example.com' is already taken

I think this behavior is reasonable as a default because merging with existing users may be a security problem if the admin isn't careful.

This patch adds a config option to allow using existing users. This seems to work for me but needs more testing.

What do you think? Is this a good solution? Are there any better ways to migrate from LDAP (or other password login) to OIDC?

[richvdh]

I'm a bit concerned that you might end up accidentally getting false matches. A better way might be if you could get a list of the sub values from your OpenID Connect provider, so that you can prepopulate the user_external_ids table for any existing users?

[BBBSnowball]

I'm migrating from LDAP to OpenID Connect so there shouldn't be any chance of false matches. I do agree that merging shouldn't be the default behavior. We mustn't merge users unless the admin says it is safe to do so.

In fact, I do have a local admin account on the production server. This might become a false match. However, I will have to migrate, delete or disable this account anyway because Riot won't let me do password login if SSO is enabled.

I don't know of any official way to get sub values from Gitlab. I had assumed that they are opaque tokens that are generated when the user uses OIDC for the first time. However, this doesn't seem to be so. Evidential evidence suggests that Gitlab is using its internal user id. If this is so, I can get (most of) the list from the database (SELECT id,username FROM users).

This feels like a work-around but I think I can make it work. If I'm the only one interested in this, there isn't enough justification for adding code to Synapse to handle my use case.

There are some downsides to this approach:

1. It needs database admin permissions whereas a normal user can add an app with OIDC permissions. Not a problem for me.
2. Gitlab and Synapse create the user on first login. That means that there will be some users that exist in Synapse but not in Gitlab. There shouldn't be too many of these so I can fix them manually as soon as they complain that their account doesn't work anymore.
3. If Gitlab doesn't work as I guessed (or changes in a future version or for old users), there might be false matches. I can do some reading of source code but all bets are off when we messing with internals. I'm running this for a small community so I'm not too worried. If anyone wants to do this for an important server, please be careful.

I think I will rather keep my local patch and ask users to login before the next upgrade.

Should I close the issue now or wait for a few days whether more users are interested in this?

[richvdh]

ok fair enough.

I think if we were to land this we'd want to iterate a bit on things like the name of the config setting. my inclination would be to leave it for now and see if other people would find it useful

[hungrymonkey]

I am here to post the commands for the workaround. Feel free to critique my comment.

auth_provider	external_id	user_id
saml	username2	@username2:domain.com
saml	username3	@Username3:domain.com
saml	username4	@username4:domain.com
oidc	d853dfd2-1917-4d0s-879e-c9ba674d4b70	@username5:domain.com
oidc	5ee566z9-6b6e-4ac6-op53-9a2c614sd902	@username6:domain.com

UPDATE user_external_ids SET auth_provider = 'oidc', external_id = 'openid-uuid' WHERE user_id like '@username3:domain';

[BBBSnowball]

This will work if there is already a row for that user in user_external_ids. This table doesn't seem to be used for LDAP. Therefore, I need something like this:

INSERT INTO user_external_ids VALUES('oidc','openid-uuid','@username2:domain.com');

Please have a look at your database to determine whether you need the UPDATE or INSERT statement. As there isn't any unique constraint on the user_id, don't run the INSERT statement without checking. Furthermore, you should only add those rows for users that already exist in Synapse. You can check like this:

select name from users where name = '@username2:domain.com';

Please note that openid-uuid is chosen by the identity provider and some providers are using different strings. For example, Gitlab seem to use its internal user id converted to a string so this would be something like '42'.

I'm closing this because I have a workaround (my local patch) and there haven't been any further requests for this feature by other users.

[haslersn]

I need this feature.

[hungrymonkey]

@BBBSnowball I am currently using an IdP to hide implementation details like LDAP or google logins, but I noticed my IdP seems to change the uuid when I attempted to restore it after nuking it. I guess the only way to make it stable is to back up the db.

[BBBSnowball]

I need this feature.

My patch should still work if you remove the hunks that change the example config.

[...] I noticed my IdP seems to change the uuid when I attempted to restore it after nuking it.

Yeah, I think you should backup the db in that case. My patch might seem to work but Synapse would accumulate several uuids for the same user. That may cause problems - and it might even fail immediately if the db has uniqueness constraints (which it probably does).

If nuking the db is not a regular thing but for desaster recovery only (e.g. server died and backup cannot be restored), you should be able to recover by nuking the mappings in both the IdP and Synapse (user_external_ids). Then, populate it by the methods decribed above or use my patch. Backup of the IdP database should be a lot easier, of course.

[BBBSnowball]

[...] if the db has uniqueness constraints

It doesn't have a unique constraint for user_id so it shouldn't fail outright (see source code. The table is only used in a few places and those shouldn't have any problem with duplicate user_ids.

One obvious failure mode would be a random match with an old uuid. That shouldn't happen if the IdP uses actual uuids. If it doesn't, this may be a source of security incidents.

My patch is also relying on that behavior, of course. In that case, there cannot be any random matches with old data because the IdP (auth_provider) will be different.

[andrewperry]

This would definitely be a useful feature, moving from the old ldap method with the rest_auth_provider, where there is no user_external_ids it would be nice if it would enable you to link them!