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

On login with external user ID (Oauth2) ensure an external login is created #23501

Closed
wants to merge 1 commit into from

Conversation

zeripath
Copy link
Contributor

Gitea has an external login user table which only gets used for linked accounts. This appears to be a longstanding mistake and a bug, as they should be created for all external users.

Close #1143

…reated

Gitea has an external login user table which only gets used for linked
accounts. This appears to bea longstanding mistake and a bug, as they
should be created for all external users.

Signed-off-by: Andrew Thornton <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #23501 (f971cd1) into main (f521e88) will decrease coverage by 0.02%.
The diff coverage is 42.63%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main   #23501      +/-   ##
==========================================
- Coverage   47.14%   47.12%   -0.02%     
==========================================
  Files        1149     1154       +5     
  Lines      151446   152198     +752     
==========================================
+ Hits        71397    71726     +329     
- Misses      71611    72002     +391     
- Partials     8438     8470      +32     
Impacted Files Coverage Δ
cmd/dump.go 0.67% <0.00%> (ø)
models/actions/run.go 1.64% <0.00%> (-0.08%) ⬇️
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
models/user/search.go 77.50% <0.00%> (-6.29%) ⬇️
modules/actions/github.go 0.00% <0.00%> (ø)
modules/actions/workflows.go 0.00% <0.00%> (ø)
modules/doctor/storage.go 31.93% <0.00%> (ø)
modules/setting/git.go 45.45% <ø> (ø)
modules/storage/minio.go 1.51% <0.00%> (-0.06%) ⬇️
modules/structs/user.go 100.00% <ø> (ø)
... and 35 more

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 15, 2023
Comment on lines +1141 to +1142
}
if !errors.Is(err, util.ErrAlreadyExist) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
if !errors.Is(err, util.ErrAlreadyExist) {
if err != nil && !errors.Is(err, util.ErrAlreadyExist) {
log.Error("LinkAccountToUser failed: %v", err)
}
} else {

@wxiaoguang
Copy link
Contributor

I guess we go back to history:

That may not be correct. If I understand the existing code an ExternalLoginUser

makes the connecting between some existing user and additional external login sources

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 User.LoginType has a specific value and no ExternalLoginUser is needed. (?)

ps: I haven't read the code and don't understand the problem, it's just unclear to me about what the design was and what's the expected behavior.

@zeripath zeripath closed this May 7, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants