From bffbbf547063fa170cc52ae2e757d5badb336632 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 25 Apr 2024 19:22:32 +0800 Subject: Improve oauth2 client "preferred username field" logic and the error handling (#30622) Follow #30454 And fix #24957 When using "preferred_username", if no such field, `extractUserNameFromOAuth2` (old `getUserName`) shouldn't return an error. All other USERNAME options do not return such error. And fine tune some logic and error messages, make code more stable and more friendly to end users. --- routers/web/auth/auth.go | 14 +++++++------- routers/web/auth/auth_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ routers/web/auth/linkaccount.go | 20 ++++++++++++-------- routers/web/auth/oauth.go | 35 ++++++++++++++++++++++------------- 4 files changed, 81 insertions(+), 28 deletions(-) (limited to 'routers/web/auth') diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 9ef32ebdb1..7c873796fe 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -382,17 +382,17 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe return setting.AppSubURL + "/" } -func getUserName(gothUser *goth.User) (string, error) { +// extractUserNameFromOAuth2 tries to extract a normalized username from the given OAuth2 user. +// It returns ("", nil) if the required field doesn't exist. +func extractUserNameFromOAuth2(gothUser *goth.User) (string, error) { switch setting.OAuth2Client.Username { case setting.OAuth2UsernameEmail: - return user_model.NormalizeUserName(strings.Split(gothUser.Email, "@")[0]) + return user_model.NormalizeUserName(gothUser.Email) case setting.OAuth2UsernamePreferredUsername: - preferredUsername, exists := gothUser.RawData["preferred_username"] - if exists { - return user_model.NormalizeUserName(preferredUsername.(string)) - } else { - return "", fmt.Errorf("preferred_username is missing in received user data but configured as username source for user_id %q. Check if OPENID_CONNECT_SCOPES contains profile", gothUser.UserID) + if preferredUsername, ok := gothUser.RawData["preferred_username"].(string); ok { + return user_model.NormalizeUserName(preferredUsername) } + return "", nil case setting.OAuth2UsernameNickname: return user_model.NormalizeUserName(gothUser.NickName) default: // OAuth2UsernameUserid diff --git a/routers/web/auth/auth_test.go b/routers/web/auth/auth_test.go index c6afbf877c..45525a5c6f 100644 --- a/routers/web/auth/auth_test.go +++ b/routers/web/auth/auth_test.go @@ -8,12 +8,31 @@ import ( "net/url" "testing" + auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/modules/session" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/services/auth/source/oauth2" "code.gitea.io/gitea/services/contexttest" + "github.com/markbates/goth" + "github.com/markbates/goth/gothic" "github.com/stretchr/testify/assert" ) +func addOAuth2Source(t *testing.T, authName string, cfg oauth2.Source) { + cfg.Provider = util.IfZero(cfg.Provider, "gitea") + err := auth_model.CreateSource(db.DefaultContext, &auth_model.Source{ + Type: auth_model.OAuth2, + Name: authName, + IsActive: true, + Cfg: &cfg, + }) + assert.NoError(t, err) +} + func TestUserLogin(t *testing.T) { ctx, resp := contexttest.MockContext(t, "/user/login") SignIn(ctx) @@ -41,3 +60,24 @@ func TestUserLogin(t *testing.T) { SignIn(ctx) assert.Equal(t, "/", test.RedirectURL(resp)) } + +func TestSignUpOAuth2ButMissingFields(t *testing.T) { + defer test.MockVariableValue(&setting.OAuth2Client.EnableAutoRegistration, true)() + defer test.MockVariableValue(&gothic.CompleteUserAuth, func(res http.ResponseWriter, req *http.Request) (goth.User, error) { + return goth.User{Provider: "dummy-auth-source", UserID: "dummy-user"}, nil + })() + + addOAuth2Source(t, "dummy-auth-source", oauth2.Source{}) + + mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockStore("dummy-sid")} + ctx, resp := contexttest.MockContext(t, "/user/oauth2/dummy-auth-source/callback?code=dummy-code", mockOpt) + ctx.SetParams("provider", "dummy-auth-source") + SignInOAuthCallback(ctx) + assert.Equal(t, http.StatusSeeOther, resp.Code) + assert.Equal(t, "/user/link_account", test.RedirectURL(resp)) + + // then the user will be redirected to the link account page, and see a message about the missing fields + ctx, _ = contexttest.MockContext(t, "/user/link_account", mockOpt) + LinkAccount(ctx) + assert.EqualValues(t, "auth.oauth_callback_unable_auto_reg:dummy-auth-source,email", ctx.Data["AutoRegistrationFailedPrompt"]) +} diff --git a/routers/web/auth/linkaccount.go b/routers/web/auth/linkaccount.go index f744a57a43..24130df634 100644 --- a/routers/web/auth/linkaccount.go +++ b/routers/web/auth/linkaccount.go @@ -48,23 +48,27 @@ func LinkAccount(ctx *context.Context) { ctx.Data["SignInLink"] = setting.AppSubURL + "/user/link_account_signin" ctx.Data["SignUpLink"] = setting.AppSubURL + "/user/link_account_signup" - gothUser := ctx.Session.Get("linkAccountGothUser") - if gothUser == nil { - ctx.ServerError("UserSignIn", errors.New("not in LinkAccount session")) + gothUser, ok := ctx.Session.Get("linkAccountGothUser").(goth.User) + if !ok { + // no account in session, so just redirect to the login page, then the user could restart the process + ctx.Redirect(setting.AppSubURL + "/user/login") return } - gu, _ := gothUser.(goth.User) - uname, err := getUserName(&gu) + if missingFields, ok := gothUser.RawData["__giteaAutoRegMissingFields"].([]string); ok { + ctx.Data["AutoRegistrationFailedPrompt"] = ctx.Tr("auth.oauth_callback_unable_auto_reg", gothUser.Provider, strings.Join(missingFields, ",")) + } + + uname, err := extractUserNameFromOAuth2(&gothUser) if err != nil { ctx.ServerError("UserSignIn", err) return } - email := gu.Email + email := gothUser.Email ctx.Data["user_name"] = uname ctx.Data["email"] = email - if len(email) != 0 { + if email != "" { u, err := user_model.GetUserByEmail(ctx, email) if err != nil && !user_model.IsErrUserNotExist(err) { ctx.ServerError("UserSignIn", err) @@ -73,7 +77,7 @@ func LinkAccount(ctx *context.Context) { if u != nil { ctx.Data["user_exists"] = true } - } else if len(uname) != 0 { + } else if uname != "" { u, err := user_model.GetUserByName(ctx, uname) if err != nil && !user_model.IsErrUserNotExist(err) { ctx.ServerError("UserSignIn", err) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 3189d1372e..c9cb7859cd 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -934,7 +934,7 @@ func SignInOAuthCallback(ctx *context.Context) { if u == nil { if ctx.Doer != nil { - // attach user to already logged in user + // attach user to the current signed-in user err = externalaccount.LinkAccountToUser(ctx, ctx.Doer, gothUser) if err != nil { ctx.ServerError("UserLinkAccount", err) @@ -952,21 +952,30 @@ func SignInOAuthCallback(ctx *context.Context) { if gothUser.Email == "" { missingFields = append(missingFields, "email") } - if setting.OAuth2Client.Username == setting.OAuth2UsernameNickname && gothUser.NickName == "" { - missingFields = append(missingFields, "nickname") + uname, err := extractUserNameFromOAuth2(&gothUser) + if err != nil { + ctx.ServerError("UserSignIn", err) + return + } + if uname == "" { + if setting.OAuth2Client.Username == setting.OAuth2UsernameNickname { + missingFields = append(missingFields, "nickname") + } else if setting.OAuth2Client.Username == setting.OAuth2UsernamePreferredUsername { + missingFields = append(missingFields, "preferred_username") + } // else: "UserID" and "Email" have been handled above separately } if len(missingFields) > 0 { - log.Error("OAuth2 Provider %s returned empty or missing fields: %s", authSource.Name, missingFields) - if authSource.IsOAuth2() && authSource.Cfg.(*oauth2.Source).Provider == "openidConnect" { - log.Error("You may need to change the 'OPENID_CONNECT_SCOPES' setting to request all required fields") + log.Error(`OAuth2 auto registration (ENABLE_AUTO_REGISTRATION) is enabled but OAuth2 provider %q doesn't return required fields: %s. `+ + `Suggest to: disable auto registration, or make OPENID_CONNECT_SCOPES (for OpenIDConnect) / Authentication Source Scopes (for Admin panel) to request all required fields, and the fields shouldn't be empty.`, + authSource.Name, strings.Join(missingFields, ",")) + // The RawData is the only way to pass the missing fields to the another page at the moment, other ways all have various problems: + // by session or cookie: difficult to clean or reset; by URL: could be injected with uncontrollable content; by ctx.Flash: the link_account page is a mess ... + // Since the RawData is for the provider's data, so we need to use our own prefix here to avoid conflict. + if gothUser.RawData == nil { + gothUser.RawData = make(map[string]any) } - err = fmt.Errorf("OAuth2 Provider %s returned empty or missing fields: %s", authSource.Name, missingFields) - ctx.ServerError("CreateUser", err) - return - } - uname, err := getUserName(&gothUser) - if err != nil { - ctx.ServerError("UserSignIn", err) + gothUser.RawData["__giteaAutoRegMissingFields"] = missingFields + showLinkingLogin(ctx, gothUser) return } u = &user_model.User{ -- cgit v1.2.3