Improve oauth2 client "preferred username field" logic and the error handling (#30622)

wxiaoguang · web-flow · commit bffbbf547063 · 2024-04-25T11:22:32.000Z
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.
diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini
@@ -1558,8 +1558,8 @@ LEVEL = Info
 ;; email = use the username part of the email attribute
 ;; Note: `nickname`, `preferred_username` and `email` options will normalize input strings using the following criteria:
 ;; - diacritics are removed
-;; - the characters in the set `['´\x60]` are removed
-;; - the characters in the set `[\s~+]` are replaced with `-`
+;; - the characters in the set ['´`] are removed
+;; - the characters in the set [\s~+] are replaced with "-"
 ;USERNAME = nickname
 ;;
 ;; Update avatar if available from oauth2 provider.
diff --git a/docs/content/administration/config-cheat-sheet.en-us.md b/docs/content/administration/config-cheat-sheet.en-us.md
@@ -612,7 +612,7 @@ And the following unique queues:
   - `email` - use the username part of the email attribute
   - Note: `nickname`, `preferred_username` and `email` options will normalize input strings using the following criteria:
     - diacritics are removed
-    - the characters in the set `['´\x60]` are removed
+    - the characters in the set ```['´`]``` are removed
     - the characters in the set `[\s~+]` are replaced with `-`
 - `UPDATE_AVATAR`: **false**: Update avatar if available from oauth2 provider. Update will be performed on each login.
 - `ACCOUNT_LINKING`: **login**: How to handle if an account / email already exists:
diff --git a/models/unittest/testdb.go b/models/unittest/testdb.go
@@ -16,6 +16,7 @@ import (
 	"code.gitea.io/gitea/models/system"
 	"code.gitea.io/gitea/modules/auth/password/hash"
 	"code.gitea.io/gitea/modules/base"
+	"code.gitea.io/gitea/modules/cache"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/setting/config"
@@ -106,6 +107,7 @@ func MainTest(m *testing.M, testOpts ...*TestOptions) {
 		fatalTestError("Error creating test engine: %v\n", err)
 	}
 
+	setting.IsInTesting = true
 	setting.AppURL = "https://try.gitea.io/"
 	setting.RunUser = "runuser"
 	setting.SSH.User = "sshuser"
@@ -148,6 +150,9 @@ func MainTest(m *testing.M, testOpts ...*TestOptions) {
 
 	config.SetDynGetter(system.NewDatabaseDynKeyGetter())
 
+	if err = cache.Init(); err != nil {
+		fatalTestError("cache.Init: %v\n", err)
+	}
 	if err = storage.Init(); err != nil {
 		fatalTestError("storage.Init: %v\n", err)
 	}
diff --git a/models/user/user.go b/models/user/user.go
@@ -501,19 +501,19 @@ func GetUserSalt() (string, error) {
 // Note: The set of characters here can safely expand without a breaking change,
 // but characters removed from this set can cause user account linking to break
 var (
-	customCharsReplacement    = strings.NewReplacer("Æ", "AE")
-	removeCharsRE             = regexp.MustCompile(`['´\x60]`)
-	removeDiacriticsTransform = transform.Chain(norm.NFD, runes.Remove(runes.In(unicode.Mn)), norm.NFC)
-	replaceCharsHyphenRE      = regexp.MustCompile(`[\s~+]`)
+	customCharsReplacement = strings.NewReplacer("Æ", "AE")
+	removeCharsRE          = regexp.MustCompile("['`´]")
+	transformDiacritics    = transform.Chain(norm.NFD, runes.Remove(runes.In(unicode.Mn)), norm.NFC)
+	replaceCharsHyphenRE   = regexp.MustCompile(`[\s~+]`)
 )
 
-// normalizeUserName returns a string with single-quotes and diacritics
-// removed, and any other non-supported username characters replaced with
-// a `-` character
+// NormalizeUserName only takes the name part if it is an email address, transforms it diacritics to ASCII characters.
+// It returns a string with the single-quotes removed, and any other non-supported username characters are replaced with a `-` character
 func NormalizeUserName(s string) (string, error) {
-	strDiacriticsRemoved, n, err := transform.String(removeDiacriticsTransform, customCharsReplacement.Replace(s))
+	s, _, _ = strings.Cut(s, "@")
+	strDiacriticsRemoved, n, err := transform.String(transformDiacritics, customCharsReplacement.Replace(s))
 	if err != nil {
-		return "", fmt.Errorf("Failed to normalize character `%v` in provided username `%v`", s[n], s)
+		return "", fmt.Errorf("failed to normalize the string of provided username %q at position %d", s, n)
 	}
 	return replaceCharsHyphenRE.ReplaceAllLiteralString(removeCharsRE.ReplaceAllLiteralString(strDiacriticsRemoved, ""), "-"), nil
 }
diff --git a/models/user/user_test.go b/models/user/user_test.go
@@ -506,15 +506,16 @@ func Test_NormalizeUserFromEmail(t *testing.T) {
 		Expected          string
 		IsNormalizedValid bool
 	}{
-		{"test", "test", true},
+		{"name@example.com", "name", true},
+		{"test'`´name", "testname", true},
 		{"Sinéad.O'Connor", "Sinead.OConnor", true},
 		{"Æsir", "AEsir", true},
-		// \u00e9\u0065\u0301
-		{"éé", "ee", true},
+		{"éé", "ee", true}, // \u00e9\u0065\u0301
 		{"Awareness Hub", "Awareness-Hub", true},
 		{"double__underscore", "double__underscore", false}, // We should consider squashing double non-alpha characters
 		{".bad.", ".bad.", false},
 		{"new😀user", "new😀user", false}, // No plans to support
+		{`"quoted"`, `"quoted"`, false}, // No plans to support
 	}
 	for _, testCase := range testCases {
 		normalizedName, err := user_model.NormalizeUserName(testCase.Input)
diff --git a/modules/session/mock.go b/modules/session/mock.go
@@ -0,0 +1,26 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package session
+
+import (
+	"net/http"
+
+	"gitea.com/go-chi/session"
+)
+
+type MockStore struct {
+	*session.MemStore
+}
+
+func (m *MockStore) Destroy(writer http.ResponseWriter, request *http.Request) error {
+	return nil
+}
+
+type mockStoreContextKeyStruct struct{}
+
+var MockStoreContextKey = mockStoreContextKeyStruct{}
+
+func NewMockStore(sid string) *MockStore {
+	return &MockStore{session.NewMemStore(sid)}
+}
diff --git a/modules/session/store.go b/modules/session/store.go
@@ -6,6 +6,8 @@ package session
 import (
 	"net/http"
 
+	"code.gitea.io/gitea/modules/setting"
+
 	"gitea.com/go-chi/session"
 )
 
@@ -14,15 +16,32 @@ type Store interface {
 	Get(any) any
 	Set(any, any) error
 	Delete(any) error
+	ID() string
+	Release() error
+	Flush() error
+	Destroy(http.ResponseWriter, *http.Request) error
 }
 
 // RegenerateSession regenerates the underlying session and returns the new store
 func RegenerateSession(resp http.ResponseWriter, req *http.Request) (Store, error) {
 	for _, f := range BeforeRegenerateSession {
 		f(resp, req)
 	}
-	s, err := session.RegenerateSession(resp, req)
-	return s, err
+	if setting.IsInTesting {
+		if store, ok := req.Context().Value(MockStoreContextKey).(*MockStore); ok {
+			return store, nil
+		}
+	}
+	return session.RegenerateSession(resp, req)
+}
+
+func GetContextSession(req *http.Request) Store {
+	if setting.IsInTesting {
+		if store, ok := req.Context().Value(MockStoreContextKey).(*MockStore); ok {
+			return store
+		}
+	}
+	return session.GetSession(req)
 }
 
 // BeforeRegenerateSession is a list of functions that are called before a session is regenerated.
diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go
@@ -16,14 +16,10 @@ import (
 type OAuth2UsernameType string
 
 const (
-	// OAuth2UsernameUserid oauth2 userid field will be used as gitea name
-	OAuth2UsernameUserid OAuth2UsernameType = "userid"
-	// OAuth2UsernameNickname oauth2 nickname field will be used as gitea name
-	OAuth2UsernameNickname OAuth2UsernameType = "nickname"
-	// OAuth2UsernameEmail username of oauth2 email field will be used as gitea name
-	OAuth2UsernameEmail OAuth2UsernameType = "email"
-	// OAuth2UsernameEmail username of oauth2 preferred_username field will be used as gitea name
-	OAuth2UsernamePreferredUsername OAuth2UsernameType = "preferred_username"
+	OAuth2UsernameUserid            OAuth2UsernameType = "userid"             // use user id (sub) field as gitea's username
+	OAuth2UsernameNickname          OAuth2UsernameType = "nickname"           // use nickname field
+	OAuth2UsernameEmail             OAuth2UsernameType = "email"              // use email field
+	OAuth2UsernamePreferredUsername OAuth2UsernameType = "preferred_username" // use preferred_username field
 )
 
 func (username OAuth2UsernameType) isValid() bool {
@@ -71,8 +67,8 @@ func loadOAuth2ClientFrom(rootCfg ConfigProvider) {
 	OAuth2Client.EnableAutoRegistration = sec.Key("ENABLE_AUTO_REGISTRATION").MustBool()
 	OAuth2Client.Username = OAuth2UsernameType(sec.Key("USERNAME").MustString(string(OAuth2UsernameNickname)))
 	if !OAuth2Client.Username.isValid() {
-		log.Warn("Username setting is not valid: '%s', will fallback to '%s'", OAuth2Client.Username, OAuth2UsernameNickname)
 		OAuth2Client.Username = OAuth2UsernameNickname
+		log.Warn("[oauth2_client].USERNAME setting is invalid, falls back to %q", OAuth2Client.Username)
 	}
 	OAuth2Client.UpdateAvatar = sec.Key("UPDATE_AVATAR").MustBool()
 	OAuth2Client.AccountLinking = OAuth2AccountLinkingType(sec.Key("ACCOUNT_LINKING").MustString(string(OAuth2AccountLinkingLogin)))
diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
@@ -436,6 +436,7 @@ oauth_signin_submit = Link Account
 oauth.signin.error = There was an error processing the authorization request. If this error persists, please contact the site administrator.
 oauth.signin.error.access_denied = The authorization request was denied.
 oauth.signin.error.temporarily_unavailable = Authorization failed because the authentication server is temporarily unavailable. Please try again later.
+oauth_callback_unable_auto_reg = Auto Registration is enabled, but OAuth2 Provider %[1]s returned missing fields: %[2]s, unable to create an account automatically, please create or link to an account, or contact the site administrator.
 openid_connect_submit = Connect
 openid_connect_title = Connect to an existing account
 openid_connect_desc = The chosen OpenID URI is unknown. Associate it with a new account here.
diff --git 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
@@ -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
@@ -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
@@ -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{
diff --git a/services/context/context.go b/services/context/context.go
diff --git a/services/contexttest/context_tests.go b/services/contexttest/context_tests.go
diff --git a/templates/user/auth/link_account.tmpl b/templates/user/auth/link_account.tmpl
