fix(fly): return user with correct auth #56523
Conversation
github-actions
bot
added
the
Scope: Backend
| for candid_user in user_query: | ||
| identity_query = AuthIdentity.objects.filter( | ||
| user=candid_user, ident=ident | ||
| ) |
There was a problem hiding this comment.
Looks like an N+1 query? Probably want to do something like
AuthIdentity.objects.filter(user__in=user_query, ident=ident)and then loop over the result.
It might also be worth logging if that query returns multiple AuthIdentity objects.
Codecov Report
@@ Coverage Diff @@
## master #56523 +/- ##
==========================================
- Coverage 78.64% 78.62% -0.02%
==========================================
Files 5078 5083 +5
Lines 218266 219131 +865
Branches 36952 37104 +152
==========================================
+ Hits 171646 172291 +645
- Misses 41085 41265 +180
- Partials 5535 5575 +40
|
| @@ -172,7 +174,9 @@ def get_first_superuser(self) -> Optional[RpcUser]: | |||
| return None | |||
| return serialize_rpc_user(user) | |||
|
|
|||
| def get_or_create_user_by_email(self, *, email: str) -> RpcUser: | |||
| def get_or_create_user_by_email( | |||
There was a problem hiding this comment.
@sentaur-athena Since get_or_create_user_by_email() is called in getsentry, and the function signature has changed, it might be better to use a new function name (e.g. get_or_create_user_by_email2) so that it's easier to make the switch on getsentry.
Otherwise, the get_or_create_user_by_email() call will error out on getsentry.
There was a problem hiding this comment.
Interesting. @dashed would it still error although I defined the new parameters as optional? It doesn't error locally without changes in getsentry
There was a problem hiding this comment.
@sentaur-athena I think it'll be fine if the new arguments are optional.
| user = user_query[0] | ||
| if ident: | ||
| for candid_user in user_query: | ||
| identity_query = AuthIdentity.objects.filter( |
There was a problem hiding this comment.
@sentaur-athena This might not work.
We call auth_service.create_auth_identity() after get_or_create_new_user():
provision_with_new_org(): https://github.com/getsentry/getsentry/blob/db817ebe096f2f807a52203079760c2aba31c550/getsentry/web/channel_provisioning/process.py#L281-L286provision_for_existing_org(): https://github.com/getsentry/getsentry/blob/db817ebe096f2f807a52203079760c2aba31c550/getsentry/web/channel_provisioning/process.py#L304-L310
There was a problem hiding this comment.
@dashed I'm aware of that. The problem I'm facing is for the people who have been in this flow before and already have auth_identity created for them. For example:
- Athena has 2 users, Athena1 and Athena2
- She provisions a new org and in that process we create Athena1 an auth identity related to that org
- Later she provisions a project get_or_create_new_user() returns Athena2 so then auth_service.create_auth_identity() tries to create auth_identity because Athena2 doesn't have one. That would throw an error because Athena1 is using the same ident.
In this fix I want to make sure that if a user already has an ident return that one from get_or_create_new_user() otherwise it doesn't matter.
There was a problem hiding this comment.
@sentaur-athena that makes sense. The case you outlined might be good to add to https://github.com/getsentry/getsentry/blob/master/tests/getsentry/web/channel_provisioning/test_process.py in case you haven't done so. 👍
When we find 2 users for one email we return the first one so the user experience doesn't break. This is causing issue for fly because one of the users is the one that's setup with fly auth. This fix returns the user who has the auth setup or the first user if ident is None
When we find 2 users for one email we return the first one so the user experience doesn't break. This is causing issue for fly because one of the users is the one that's setup with fly auth.
This fix returns the user who has the auth setup or the first user if ident is None