siemens / sentry-auth-oidc

OpenID Connect SSO provider for Sentry
Apache License 2.0
69 stars 28 forks source link

Duplicate users when upgrading Sentry #29

Closed esantoro closed 3 years ago

esantoro commented 3 years ago

Hello, I'm in the process of upgrading an existing Sentry instance from 9.1.2 to 21.4. In the process, I'm also switching from the old sentry-auth-gitlab plugin to sentry-auth-oidc.

The plugin works but I'm facing the following problem: when a user logs into Sentry via Gitlab, a new user is created with same e-mail, full name and data pulled from gitlab/oidc, but different internal sentry username (django username).

Thus, users logging into new sentry aren't matched back to their original projects, organizations and stuff.

From my understanding, this is due to code at https://github.com/siemens/sentry-auth-oidc/blob/master/oidc/provider.py#L116 where we'd really need to use e-mail as id instead of the account-unique sub key.

Honestly, I'm not sure why Gitlab is returning a different sub for the same user. I tried using both the old oidc app in gitlab as well as a new app, but I'm returned a different oidc sub.

Is there a way use e-mail as sub? Could this behavior made configurable ?

max-wittig commented 3 years ago

We actually face the same issues and couldn't find the cause of that. We will take a look and might just switch to Email as unique identifier. Might make sense. Thanks!

Will keep you updated here!

esantoro commented 3 years ago

Hi and thank yu for your reply.

I understand that using the oidc-provided sub was meant to avoid some other issue, but as far as I've seen by looking at comments in the code as well as past issues, a good solution could be to keep the current behavior of the code as the default execution path but provide a setting to alter such behavior.

Imho a good approach might be to use variable (similar to OIDC_CLIENT_ID, OIDC_CLIENT_SECRET, OIDC_SCOPE and OIDC_DOMAIN) to specify what property to use as user identifier.

Something like:

OIDC_CLIENT_ID = "XXXXXXXXXXXXXXXXXXXXX"
OIDC_CLIENT_SECRET = "YYYYYYYYYY"
OIDC_SCOPE = "openid email"
OIDC_DOMAIN = "https://gitlab.example.com"
OIDC_SUB_OVERRIDE = "email"

But then again, I'll be waiting for updates on your side. I not very familiar with the Sentry codebase yet so I'm not sure about the feasibility/correctness of what I'm proposing.

Thank you again!

max-wittig commented 3 years ago

I think for GitLab we should use the user_id, if possible.

esantoro commented 3 years ago

You're probably right.

max-wittig commented 3 years ago

Looking at the GitLab documentation, it seems that sub should already be the user id. https://docs.gitlab.com/ee/integration/openid_connect_provider.html#shared-information 🤔

esantoro commented 3 years ago

Indeed it's confusing to see that it seems to be correct.

I have seen that page, I'm checking and double-checking stuff around and have run out of places to bang my head against.

esantoro commented 3 years ago

I think it might be related to the sub_legacy thing that gitlab provides. The user that I tried with has been in the company for a while, and its sub/sentry user was probably create before the migration from sub_legacy to sub.

I'm going to try with a more-recently-joined colleague and see if that helps.

EDIT: when I say "for a while" in mean that I've been looking at the releases timeline and he joined before gitlab 11.1 (where sub/sub_legacy thing happend) was released.

esantoro commented 3 years ago

We can close this issue, as it seems not to really be a problem.

Turns out that Sentry has both the concept of "users" as in users of the underlying django framework, and members (as in, members of a project -- project being another sentry-concept).

Some gitlab users authenticated via odic are correctly mapped to the already-existing sentry users (and thus to the correct member) and some have a duplicate user but are then mapped to the right member (yes, this looks a bit weird).

This decreases greatly the entity of the problem for us, at least for now.

So far so good, I guess.

@max-wittig thank you very much for your support, it's been very greatly appreciated.