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

OAuth2 registered accounts should still be local #1124

Open
strk opened this Issue Mar 6, 2017 · 6 comments

Comments

3 participants
@strk
Copy link
Member

strk commented Mar 6, 2017

I think this is important to fix before 1.1.0 final.
The current code handling OAuth2 users registration encodes the OAuth2 LoginSource in the user table, while still assigning a username/password pair to the newly create user that is supposed to be usable for "local" authentication. This is an inconsistent way to use that LoginSource field because then you would not know what username/password refer to unless by adding special code to handle "OAuth2" source. Indeed this is the special code added, for example in ForgotPasswordPost:

        if !u.IsLocal() && !u.IsOAuth2() {                                      
                ctx.Data["Err_Email"] = true                                    
                ctx.RenderWithErr(ctx.Tr("auth.non_local_account"), tplForgotPassword, nil)
                return                                                          
        }      

That's weird, because the OAuth2 user's password is indeed a "local" password,
so why should there be a difference ? The code also added an IsOauth2 method to the User table, building on what seems to be a broken model.

Users should not BE local or Oauth2, credentials should be. Users can be recognized by multiple credentials, but we'd always know about them locally, so they are all local.

The inconsistency is confirmed by the fact that you can still associate an OAuth2 credential to your previosuly-registered account resulting in a "IsLocal" user with exactly the same caracteristic of the "IsOAuth2" user, except the "LoginSource" is different. Characteristics being: you can login with local username/password as well as with "OAuth2" credentials.

My suggestion here is to always keep the "LoginSource" field as an indication of the semantic for the "LoginSource" and "Password" fields in the "User" table, so to never use an OAuth2 LoginSource identifier in that table.

Can you see any drawback in that ?

\cc @willemvd

@strk

This comment has been minimized.

Copy link
Member Author

strk commented Mar 6, 2017

See also discussion in #1036
@morrildl do you have comments about this ?

@lunny lunny added the kind/bug label Mar 6, 2017

@morrildl

This comment has been minimized.

Copy link
Contributor

morrildl commented Mar 6, 2017

@strk I agree that the current behavior seems a bit weird and inconsistent. However I don't think I understand the overall login code well enough to have an opinion on what the "correct" implementation should be.

In general I definitely agree with this, though:

Users should not BE local or Oauth2, credentials should be.

@strk

This comment has been minimized.

Copy link
Member Author

strk commented Mar 6, 2017

@lunny lunny added this to the 1.x.x milestone Mar 7, 2017

strk added a commit to strk/gitea that referenced this issue Mar 7, 2017

Always put OAuth user info in the ExternalLoginUser table
Never set OAuth in User.LoginSource, which keeps referring to
the username/password interpretation. Fixes go-gitea#1124
@strk

This comment has been minimized.

Copy link
Member Author

strk commented Mar 7, 2017

@morrildl, @willemvd please see how you like the solution in #1143.
While working on it I also found that right now (before the change) you'd get NO record at all in the external_user_login table when you register with OAuth2, meaning you should not be able to unlink that credential too. With my PR you get that also fixed.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Mar 8, 2017

I agree that we have two different external login source. One is auth login source that means where we check user's password LOCAL/LDAP/SMTP/PAM should this kind. Another is link accounts, an user (include LOCAL/LDAP/SMTP/PAM user) could link to other accounts (github/google and etc.), the link type could be OAuth/OAuth2/OpenID and etc. We really need a clear document to record this.

strk added a commit to strk/gitea that referenced this issue Mar 20, 2017

Always put OAuth user info in the ExternalLoginUser table
Never set OAuth in User.LoginSource, which keeps referring to
the username/password interpretation. Fixes go-gitea#1124
@stale

This comment has been minimized.

Copy link

stale bot commented Feb 17, 2019

This issue 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 weeks. Thank you for your contributions.

@stale stale bot added the stale label Feb 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment