search

gristlabs / grist-core

Grist is the evolution of spreadsheets.
https://www.getgrist.com
Apache License 2.0
7.32k stars 326 forks source link

Using OIDC, the email is used to identify the user instead of the 'sub' claim #759

Open illode opened 1 year ago

illode commented 1 year ago

I'm running the latest (fdc3b96cf7fa) docker image, selfhosted. I wanted to use OIDC instead of dealing with SAML.

I was going to report a different bug, but wanted to change the emails of my test accounts from <mydomain> to example.com first so I could send a screenshot. After doing that and signing in, I realized it had created a new account with the same name instead of signing me in to the original account.

Multiple Test Two users in the users table of home.sqlite3: image and multiple test2@<domain> logins in the logins table of home.sqlite3: image

New personal orgs were also created, leaving the original files in limbo. At least unless I change the emails back.

All of the Test 2 entries in both screenshots are the from user in the OIDC provider (Keycloak), I just changed their emails.

As I understand it, the user should be identified using the sub claim (standard claims / ID token). The connect_id column kind of looks like it should be for that, but I'm not sure as it's all NULL.

As an aside, there were a few other issues I ran into with the selfhosted version. Should I create new issues for them, or add them to https://github.com/gristlabs/grist-core/issues/733 since it seems to have several issues in one?

paulfitz commented 1 year ago

cc @fflorent

Thanks for reporting this @illode. For other problems, generally one issue per problem is best.

fflorent commented 1 year ago

Thanks for your report @illode!

Indeed, the sub property can be used to identify uniquely a user. And you seem to be right, connect_id may be a good fit for storing this property: https://github.com/gristlabs/grist-core/blob/570e4032a416a8442d329dbe3551208a658fd6b6/app/gen-server/lib/HomeDBManager.ts#L533-L544

However, the method above seems not to be called anywhere. @paulfitz Does it make sense to take advantage of it for that purpose?

vviers commented 3 months ago

This issue came up as pretty crucial to the La Suite project, would be interested in reopening this topic :)