Improve oauth2 error handling (#33969)

Show the callback error to end users, it should be safe.
Fix #33967

options/locale/locale_en-US.ini

@@ -457,7 +457,7 @@ oauth_signup_submit = Complete Account
 oauth_signin_tab = Link to Existing Account
 oauth_signin_title = Sign In to Authorize Linked Account
 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.general = There was an error processing the authorization request: %s. 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.

routers/web/auth/auth_test.go

@@ -61,23 +61,35 @@ func TestUserLogin(t *testing.T) {
 	assert.Equal(t, "/", test.RedirectURL(resp))
 }
 
-func TestSignUpOAuth2ButMissingFields(t *testing.T) {
+func TestSignUpOAuth2Login(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")}
+	t.Run("OAuth2MissingField", func(t *testing.T) {
-	ctx, resp := contexttest.MockContext(t, "/user/oauth2/dummy-auth-source/callback?code=dummy-code", mockOpt)
+		defer test.MockVariableValue(&gothic.CompleteUserAuth, func(res http.ResponseWriter, req *http.Request) (goth.User, error) {
-	ctx.SetPathParam("provider", "dummy-auth-source")
+			return goth.User{Provider: "dummy-auth-source", UserID: "dummy-user"}, nil
-	SignInOAuthCallback(ctx)
+		})()
-	assert.Equal(t, http.StatusSeeOther, resp.Code)
+		mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockStore("dummy-sid")}
-	assert.Equal(t, "/user/link_account", test.RedirectURL(resp))
+		ctx, resp := contexttest.MockContext(t, "/user/oauth2/dummy-auth-source/callback?code=dummy-code", mockOpt)
+		ctx.SetPathParam("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
+		// 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)
+		ctx, _ = contexttest.MockContext(t, "/user/link_account", mockOpt)
-	LinkAccount(ctx)
+		LinkAccount(ctx)
-	assert.EqualValues(t, "auth.oauth_callback_unable_auto_reg:dummy-auth-source,email", ctx.Data["AutoRegistrationFailedPrompt"])
+		assert.EqualValues(t, "auth.oauth_callback_unable_auto_reg:dummy-auth-source,email", ctx.Data["AutoRegistrationFailedPrompt"])
+	})
+
+	t.Run("OAuth2CallbackError", func(t *testing.T) {
+		mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockStore("dummy-sid")}
+		ctx, resp := contexttest.MockContext(t, "/user/oauth2/dummy-auth-source/callback", mockOpt)
+		ctx.SetPathParam("provider", "dummy-auth-source")
+		SignInOAuthCallback(ctx)
+		assert.Equal(t, http.StatusSeeOther, resp.Code)
+		assert.Equal(t, "/user/login", test.RedirectURL(resp))
+		assert.Contains(t, ctx.Flash.ErrorMsg, "auth.oauth.signin.error.general")
+	})
 }

routers/web/auth/oauth.go

@@ -115,7 +115,7 @@ func SignInOAuthCallback(ctx *context.Context) {
 			case "temporarily_unavailable":
 				ctx.Flash.Error(ctx.Tr("auth.oauth.signin.error.temporarily_unavailable"))
 			default:
-				ctx.Flash.Error(ctx.Tr("auth.oauth.signin.error"))
+				ctx.Flash.Error(ctx.Tr("auth.oauth.signin.error.general", callbackErr.Description))
 			}
 			ctx.Redirect(setting.AppSubURL + "/user/login")
 			return
@@ -431,8 +431,10 @@ func oAuth2UserLoginCallback(ctx *context.Context, authSource *auth.Source, requ
 	gothUser, err := oauth2Source.Callback(request, response)
 	if err != nil {
 		if err.Error() == "securecookie: the value is too long" || strings.Contains(err.Error(), "Data too long") {
-			log.Error("OAuth2 Provider %s returned too long a token. Current max: %d. Either increase the [OAuth2] MAX_TOKEN_LENGTH or reduce the information returned from the OAuth2 provider", authSource.Name, setting.OAuth2.MaxTokenLength)
 			err = fmt.Errorf("OAuth2 Provider %s returned too long a token. Current max: %d. Either increase the [OAuth2] MAX_TOKEN_LENGTH or reduce the information returned from the OAuth2 provider", authSource.Name, setting.OAuth2.MaxTokenLength)
+			log.Error("oauth2Source.Callback failed: %v", err)
+		} else {
+			err = errCallback{Code: "internal", Description: err.Error()}
 		}
 		return nil, goth.User{}, err
 	}

tests/integration/oauth_test.go

@@ -19,7 +19,7 @@ import (
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
-	oauth2_provider "code.gitea.io/gitea/services/oauth2_provider"
+	"code.gitea.io/gitea/services/oauth2_provider"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"