fix: improve OAuth2 experience (#11715)

- fix: show oauth2 retrieve error
  - `true` indicates it only should be shown when the page is rendered
directly via `ctx.HTML` and not propagated if it redirects. As you can
see this always redirects and means the error is not shown.
  - Has the funny behavior that you get redirected to `/user/login`
without any indication what went wrong, no errors in the logs either.
- fix: pre-process OAuth2 client ID and secret
  - Spaces should are not appropriate for these input, remove them.
  - Manually copying and pasting client ID and secret from Github OAuth2
applications seems prone to introduce whitespaces.
  - The error of having a incorrect client ID is more noticeable (404 page
for the user).
  - The error of having a incorrect client secret is not noticeable (404
page for the goth library but no mention it's the wrong secret).

Reported-by: marijnh
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11715
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

routers/web/auth/oauth.go

@@ -1024,7 +1024,7 @@ func SignInOAuthCallback(ctx *context.Context) {
 			return
 		}
 		if err, ok := err.(*go_oauth2.RetrieveError); ok {
-			ctx.Flash.Error("OAuth2 RetrieveError: "+err.Error(), true)
+			ctx.Flash.Error("OAuth2 RetrieveError: " + err.Error())
 			ctx.Redirect(setting.AppSubURL + "/user/login")
 			return
 		}

services/forms/auth_form.go

@@ -57,8 +57,8 @@ type AuthenticationForm struct {
 	PAMServiceName                string
 	PAMEmailDomain                string
 	Oauth2Provider                string
-	Oauth2Key                     string
-	Oauth2Secret                  string
+	Oauth2Key                     string `preprocess:"TrimSpace"`
+	Oauth2Secret                  string `preprocess:"TrimSpace"`
 	OpenIDConnectAutoDiscoveryURL string
 	Oauth2UseCustomURL            bool
 	Oauth2TokenURL                string

tests/integration/admin_auth_source_test.go

@@ -10,6 +10,8 @@ import (
 
 	"forgejo.org/models/auth"
 	"forgejo.org/tests"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestAdminAuthAllowUsernameChangeSetting(t *testing.T) {
@@ -30,3 +32,24 @@ func TestAdminAuthAllowUsernameChangeSetting(t *testing.T) {
 
 	htmlDoc.AssertElement(t, "#allow_username_change[checked]", true)
 }
+
+func TestAdminAuthTrimSpace(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	session := loginUser(t, "user1")
+
+	source := addAuthSource(t, map[string]string{
+		"type":            fmt.Sprintf("%d", auth.OAuth2),
+		"name":            "some-name",
+		"is_active":       "on",
+		"oauth2_provider": "gitlab",
+		"oauth2_key":      " public_id  ",
+		"oauth2_secret":   "  secret_key ",
+	})
+
+	response := session.MakeRequest(t, NewRequestf(t, "GET", "/admin/auths/%d", source.ID), http.StatusOK)
+	htmlDoc := NewHTMLParser(t, response.Body)
+
+	assert.Equal(t, "public_id", htmlDoc.GetInputValueByName("oauth2_key"))
+	assert.Equal(t, "secret_key", htmlDoc.GetInputValueByName("oauth2_secret"))
+}

tests/integration/oauth_test.go

@@ -26,11 +26,13 @@ import (
 	api "forgejo.org/modules/structs"
 	"forgejo.org/modules/test"
 	"forgejo.org/routers/web/auth"
+	app_context "forgejo.org/services/context"
 	"forgejo.org/tests"
 
 	"github.com/markbates/goth"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	go_oauth2 "golang.org/x/oauth2"
 )
 
 func TestAuthorizeNoClientID(t *testing.T) {
@@ -1803,3 +1805,38 @@ func TestSignInOAuthCallbackGothUserFields(t *testing.T) {
 		assert.True(t, logFiltered[1], "Expected trace log with IDToken")
 	})
 }
+
+func TestSignInOAuthCallbackSignInRetrieveError(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	gitlabName := "gitlab"
+	gitlab := addAuthSource(t, authSourcePayloadGitLabCustom(gitlabName))
+
+	userGitLabUserID := "5678"
+	userGitLab := &user_model.User{
+		Name:        "gitlabuser",
+		Email:       "gitlabuser@example.com",
+		Passwd:      "gitlabuserpassword",
+		Type:        user_model.UserTypeIndividual,
+		LoginType:   auth_model.OAuth2,
+		LoginSource: gitlab.ID,
+		LoginName:   userGitLabUserID,
+	}
+	defer createUser(t.Context(), t, userGitLab)()
+
+	defer mockCompleteUserAuth(func(res http.ResponseWriter, req *http.Request) (goth.User, error) {
+		return goth.User{}, &go_oauth2.RetrieveError{
+			Response: &http.Response{
+				Status: "404 Not Found",
+			},
+			Body: []byte("cooked"),
+		}
+	})()
+	sess := emptyTestSession(t)
+	resp := sess.MakeRequest(t, NewRequest(t, "GET", fmt.Sprintf("/user/oauth2/%s/callback?code=XYZ&state=XYZ", gitlabName)), http.StatusSeeOther)
+
+	assert.Equal(t, "/user/login", test.RedirectURL(resp))
+	flashCookie := sess.GetCookie(app_context.CookieNameFlash)
+	assert.NotNil(t, flashCookie)
+	assert.Equal(t, "error%3DOAuth2%2BRetrieveError%253A%2Boauth2%253A%2Bcannot%2Bfetch%2Btoken%253A%2B404%2BNot%2BFound%250AResponse%253A%2Bcooked", flashCookie.Value)
+}