Create external user for new OAuth2 user #21276
Conversation
| @@ -1041,7 +1041,7 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model | |||
| } | |||
|
|
|||
| // update external user information | |||
| if err := externalaccount.UpdateExternalUser(u, gothUser); err != nil { | |||
| if err := externalaccount.UpdateExternalUser(u, gothUser, false); err != nil { | |||
There was a problem hiding this comment.
Unlike handleUserCreated, in handleOAuth2SignIn, the external user is expected to exist.
There was a problem hiding this comment.
Hmm, I tried to read the code, but the flow is quite complex.
I am not sure that handleOAuth2SignIn is guaranteed to be called after external user is prepared correctly.
Since the code was that before, I think there shouldn't be more problem. So LGTM.
There was a problem hiding this comment.
Actually, I'm not even sure the stored external user data is being used. The code couldn't store any data before, but it worked fine, just caused user concern because of error logs.
GiteaBot
added
the
lgtm/need 2
GiteaBot
added
lgtm/need 1
There was a problem hiding this comment.
Hmm... My usual comment about transactions applies here. We would actually need this to be an upsert and certainly not do a select then insert outside of a transaction.
Now, regarding this, the external user stuff seems to have gotten broken along the way here and I'm not certain whether it's being used at all or if it's helpful, or what it was intended to do.
We could merge this and then attempt to rationalize this code but I think we might want to take a closer look. Adding a little flag on to the function to say 'upsert' or not feels kinda wrong to me.
You are right. Actually, I tried to find an elegant way to fix it, but the flow is so complex. The |
|
That may not be correct. If I understand the existing code an
From the code this type is only inserted if an external login is used for an existing user but not if the user is created with/by that source. In this case
And that may be root cause: an existing |
|
Alternative fix if my assumption is correct: #21504 |
Fixes go-gitea#21202 Closes go-gitea#21276 An `ExternalLoginUser` is not mandatory if the current user account was created with/by the external login source.
Fix #21202.
After creating a new OAuth2 user, should insert an external user if it doesn't exist.