Always put OAuth user info in the ExternalLoginUser table #1143

strk · 2017-03-07T16:04:16Z

Never set OAuth in User.LoginSource, which keeps referring to
the username/password interpretation. Fixes #1124

\cc @willemvd, @morrildl, @lunny, @bkcsoft

I think this should be done before 1.1.0

lunny · 2017-03-08T00:49:18Z

It's really to need a proposal document to describe all the login.

strk · 2017-03-08T09:18:17Z

If a document is needed, it should be produced *before* introducing the OAuth2 support, rather than let it add confusion to the code base (and changes to production databases)

strk · 2017-03-08T16:06:05Z

Note that doing this after 1.1.0 is out will require a new migration to convert existing records with OAuth2 in User.LoginSource

pgaskin · 2017-03-08T19:58:15Z

@strk but even if it is done before, what happens to the installs from master builds (such as my own)?

strk · 2017-03-09T08:51:49Z

@geek1011 good question, I guess those would be just left with inconsistent database values. You might end up having users with OAuth2 LoginSource and no way to login with those OAuth2.

lunny · 2017-03-11T02:29:20Z

routers/user/auth.go

 	u := &models.User{
 		Name:        form.UserName,
 		Email:       form.Email,
 		Passwd:      form.Password,
 		IsActive:    !setting.Service.RegisterEmailConfirm,
-		LoginType:   models.LoginOAuth2,


LoginType should be plain?

The default is LoginNone, which is threated by code the same as LoginPlain.
But yes, I guess it could be LoginPlain as long as we guarantee the actual login is not of the LDAP or SMTP or PAM type...

lunny · 2017-03-11T02:30:31Z

routers/user/auth.go

@@ -648,6 +625,13 @@ func LinkAccountPostRegister(ctx *context.Context, cpt *captcha.Captcha, form au
 	}
 	log.Trace("Account created: %s", u.Name)

+	// Link the oAuth account to the user
+	err := models.LinkAccountToUser(u, gothUser.(goth.User))


How to migrate the old user?

Now that 1.1.0 is out yes, this is a problem. Too bad you didn't want to merge this before (we saw people had problems with early upgrades anyway, so this would have not been different)

Just like @geek1011 said, there many people use master, a migration needed.

lunny · 2017-03-11T02:32:35Z

I like this idea. There are two kinds of external Login Source. One is provide password authentication, another is for link.

strk · 2017-03-11T08:56:48Z

I raised this issue in the OAuth2 PR before, but my concern was not considered. Now I don't know if I have time to fix this myself. I think it should be done by the author of the OAuth2 code. It's great to be more "open" in development, but we all need to pay more attention to contributions and give proper importance to responsibility. Whoever introduces a feature should be available to maintain it.

lunny · 2017-03-11T09:06:04Z

@strk, when you review a PR, if you think your opinion should not be ignored, you could use github's request changes, then the merger should ask your confirmation before merge it. Our rules only consider two reviewers, lost this rule.

lunny · 2017-03-11T09:08:26Z

I will try to add migration for this PR when I have time.

bkcsoft · 2017-11-01T17:15:45Z

@lunny Did you have time to work on this? Otherwise I suggest we close this as "out of date"

lunny · 2017-11-02T02:00:54Z

@bkcsoft I'm afraid I have no time to work on this. I still have serval PRs in v1.3 and v1.4 need work.

stale · 2019-08-02T08:15:55Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

strk · 2019-08-06T15:50:28Z

Still relevant, not sure why Travis is sad

lafriks · 2019-08-06T15:52:18Z

As per integration tests there is error in template:

template: user/settings/account:10:45: executing "user/settings/account" at <.SignedUser.IsOAuth2>: can't evaluate field IsOAuth2 in type interface {}

Never set OAuth in User.LoginSource, which keeps referring to the username/password interpretation. Fixes go-gitea#1124

singuliere · 2022-01-02T21:27:44Z

Refactors moved routers/auth.go to routers/user/auth.go and the IsOAuth2 method from models/user.go to models/user/user.go but the logic changed by this PR remains the same. @strk it looks like your initial motivation for creating this PR is still valid, right?

strk · 2022-01-02T22:00:04Z

Yes, the motivation remains valid. Unfortunately the high version of Go required to build Gitea made me in a position to have more problems contributing (can't build w/out installing a foreign package in my otherwise all-distro-based system, and I'm short on disk space). For this reason I can't test my change much. Will gladly accept a PR to my branch if you want to work on the refactoring porting.

wxiaoguang · 2023-05-10T14:55:11Z

It's stale for petty long time.

What's the root problem behind the issue/PR?

Is there any valuable information in this context?

strk · 2023-05-10T19:57:12Z

It was mostly a design concern, a way to separate "external" authentication sources from "internal" ones. One problem being resolved would be described in #1124 (comment) but that issue was closed by stale bot too

wxiaoguang · 2023-05-10T19:59:13Z

Could some maintainers help to re-open that issue? @techknowlogick @lunny

wxiaoguang · 2023-05-24T02:14:59Z

I think this PR/issue could be kept open, until there is a clear conclusion

lunny · 2023-08-22T06:17:58Z

#1124 was opened so I think we could close this one because it's outdated.

denyskon · 2023-09-10T09:58:58Z

I also think we can close this and keep the issue open for further discussion

strk mentioned this pull request Mar 7, 2017

OAuth2 registered accounts should still be local #1124

Open

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 8, 2017

lunny added this to the 1.2.0 milestone Mar 8, 2017

lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 8, 2017

lunny reviewed Mar 11, 2017

View reviewed changes

lunny requested changes Mar 11, 2017

View reviewed changes

strk mentioned this pull request Mar 17, 2017

Add change-password admin command #1304

Merged

strk force-pushed the local-source-oauth-users branch from e882bf5 to 01d0770 Compare March 20, 2017 08:48

strk mentioned this pull request Mar 20, 2017

Deleting account results in internal server error #1145

Closed

1 task

strk mentioned this pull request Apr 18, 2017

2FA + Github login = Unable to login to existing account #1509

Closed

7 tasks

lunny modified the milestones: 1.3.0, 1.2.0 May 20, 2017

lafriks modified the milestones: 1.3.0, 1.4.0 Nov 2, 2017

lafriks modified the milestones: 1.4.0, 1.5.0 Jan 14, 2018

techknowlogick modified the milestones: 1.5.0, 1.6.0 May 25, 2018

techknowlogick removed this from the 1.6.0 milestone Aug 28, 2018

strk force-pushed the local-source-oauth-users branch from a7d5065 to b82fd19 Compare June 3, 2019 07:31

stale bot added the issue/stale label Aug 2, 2019

lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Aug 3, 2019

stale bot removed the issue/stale label Aug 3, 2019

strk force-pushed the local-source-oauth-users branch from 7e16c04 to dc03425 Compare August 8, 2019 14:41

strk added 2 commits September 20, 2019 09:07

Always put OAuth user info in the ExternalLoginUser table

b09c73b

Never set OAuth in User.LoginSource, which keeps referring to the username/password interpretation. Fixes go-gitea#1124

OAuth2 user is always local

924f5ab

strk force-pushed the local-source-oauth-users branch from dc03425 to 924f5ab Compare September 20, 2019 07:08

Update test

3e9c71e

singuliere self-requested a review January 2, 2022 21:40

zeripath mentioned this pull request Mar 15, 2023

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

Closed

wxiaoguang added issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail and removed issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented labels May 10, 2023

wxiaoguang mentioned this pull request May 20, 2023

[Summary] Improve auth source / login system #24821

Open

techknowlogick closed this May 23, 2023

wxiaoguang reopened this May 24, 2023

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 24, 2023

lunny closed this Sep 11, 2023

go-gitea locked as resolved and limited conversation to collaborators Dec 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always put OAuth user info in the ExternalLoginUser table #1143

Always put OAuth user info in the ExternalLoginUser table #1143

strk commented Mar 7, 2017

lunny commented Mar 8, 2017

strk commented Mar 8, 2017 via email

strk commented Mar 8, 2017

pgaskin commented Mar 8, 2017

strk commented Mar 9, 2017

lunny Mar 11, 2017

strk Mar 11, 2017

lunny Mar 11, 2017

strk Mar 11, 2017

lunny Mar 11, 2017

lunny commented Mar 11, 2017

strk commented Mar 11, 2017 via email

lunny commented Mar 11, 2017

lunny commented Mar 11, 2017

bkcsoft commented Nov 1, 2017

lunny commented Nov 2, 2017

stale bot commented Aug 2, 2019

strk commented Aug 6, 2019

lafriks commented Aug 6, 2019

singuliere commented Jan 2, 2022

strk commented Jan 2, 2022

wxiaoguang commented May 10, 2023

strk commented May 10, 2023

wxiaoguang commented May 10, 2023

wxiaoguang commented May 24, 2023

lunny commented Aug 22, 2023 •

edited

Loading

denyskon commented Sep 10, 2023

Always put OAuth user info in the ExternalLoginUser table #1143

Always put OAuth user info in the ExternalLoginUser table #1143

Conversation

strk commented Mar 7, 2017

lunny commented Mar 8, 2017

strk commented Mar 8, 2017 via email

strk commented Mar 8, 2017

pgaskin commented Mar 8, 2017

strk commented Mar 9, 2017

lunny Mar 11, 2017

Choose a reason for hiding this comment

strk Mar 11, 2017

Choose a reason for hiding this comment

lunny Mar 11, 2017

Choose a reason for hiding this comment

strk Mar 11, 2017

Choose a reason for hiding this comment

lunny Mar 11, 2017

Choose a reason for hiding this comment

lunny commented Mar 11, 2017

strk commented Mar 11, 2017 via email

lunny commented Mar 11, 2017

lunny commented Mar 11, 2017

bkcsoft commented Nov 1, 2017

lunny commented Nov 2, 2017

stale bot commented Aug 2, 2019

strk commented Aug 6, 2019

lafriks commented Aug 6, 2019

singuliere commented Jan 2, 2022

strk commented Jan 2, 2022

wxiaoguang commented May 10, 2023

strk commented May 10, 2023

wxiaoguang commented May 10, 2023

wxiaoguang commented May 24, 2023

lunny commented Aug 22, 2023 • edited Loading

denyskon commented Sep 10, 2023

lunny commented Aug 22, 2023 •

edited

Loading