Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
illode opened this issue Nov 20, 2023 · 3 comments
Assignees
Labels

Comments

@illode
Copy link

illode commented Nov 20, 2023

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 #733 since it seems to have several issues in one?

@paulfitz
Copy link
Member

paulfitz commented Nov 21, 2023

cc @fflorent

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

@fflorent
Copy link
Collaborator

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:

/**
* Ensures that user with external id exists and updates its profile and email if necessary.
*
* @param profile External profile
*/
public async ensureExternalUser(profile: UserProfile) {
await this._connection.transaction(async manager => {
// First find user by the connectId from the profile
const existing = await manager.findOne(User, {
where: {connectId: profile.connectId || undefined},
relations: ["logins"],
});

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

fflorent pushed a commit to incubateur-territoires/grist-core that referenced this issue Nov 24, 2023
Using the 'sub' claim allows the user to change their email
in the OIDC Identity Provider without loosing their work.
@vviers
Copy link
Collaborator

vviers commented Sep 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

5 participants