OAuth2 registered accounts should still be local by using ExternalLoginUser

[strk]

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]

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

[morrildl]

@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]

I think the current rationale for a LoginSource to even exist in the User table is because when you login with username/password the code checks if the user (by username) is known and if it is delegates password checking to the asociated LoginSource. Code is in models/login_source.go within the UserSignIn function. You can see how the inconsistency of OAuth2 can also be seen there, around here: ``` switch user.LoginType { case LoginNoType, LoginPlain, LoginOAuth2: if user.ValidatePassword(password) { return user, nil } ``` ... doesn't make sense to me to call user.ValiatePassword for a user with LoginOAuth2 source, does it ?

[strk]

@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]

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.

[stale]

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]

This issue has been automatically closed because of inactivity. You can re-open it if needed.