Unify two factor check (#27915)

modules/context/api.go

@@ -11,7 +11,6 @@ import (
 	"net/url"
 	"strings"
 
-	"code.gitea.io/gitea/models/auth"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
@@ -197,32 +196,6 @@ func (ctx *APIContext) SetLinkHeader(total, pageSize int) {
 	}
 }
 
-// CheckForOTP validates OTP
-func (ctx *APIContext) CheckForOTP() {
-	if skip, ok := ctx.Data["SkipLocalTwoFA"]; ok && skip.(bool) {
-		return // Skip 2FA
-	}
-
-	otpHeader := ctx.Req.Header.Get("X-Gitea-OTP")
-	twofa, err := auth.GetTwoFactorByUID(ctx.Doer.ID)
-	if err != nil {
-		if auth.IsErrTwoFactorNotEnrolled(err) {
-			return // No 2FA enrollment for this user
-		}
-		ctx.Error(http.StatusInternalServerError, "GetTwoFactorByUID", err)
-		return
-	}
-	ok, err := twofa.ValidateTOTP(otpHeader)
-	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "ValidateTOTP", err)
-		return
-	}
-	if !ok {
-		ctx.Error(http.StatusUnauthorized, "", nil)
-		return
-	}
-}
-
 // APIContexter returns apicontext as middleware
 func APIContexter() func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {

routers/api/v1/api.go

@@ -315,10 +315,6 @@ func reqToken() func(ctx *context.APIContext) {
 			return
 		}
 
-		if ctx.IsBasicAuth {
-			ctx.CheckForOTP()
-			return
-		}
 		if ctx.IsSigned {
 			return
 		}
@@ -340,7 +336,6 @@ func reqBasicAuth() func(ctx *context.APIContext) {
 			ctx.Error(http.StatusUnauthorized, "reqBasicAuth", "auth required")
 			return
 		}
-		ctx.CheckForOTP()
 	}
 }
 
@@ -687,12 +682,6 @@ func bind[T any](_ T) any {
 	}
 }
 
-// The OAuth2 plugin is expected to be executed first, as it must ignore the user id stored
-// in the session (if there is a user id stored in session other plugins might return the user
-// object for that id).
-//
-// The Session plugin is expected to be executed second, in order to skip authentication
-// for users that have already signed in.
 func buildAuthGroup() *auth.Group {
 	group := auth.NewGroup(
 		&auth.OAuth2{},

services/auth/basic.go

@@ -15,6 +15,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web/middleware"
 )
 
@@ -132,11 +133,30 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore
 		return nil, err
 	}
 
-	if skipper, ok := source.Cfg.(LocalTwoFASkipper); ok && skipper.IsSkipLocalTwoFA() {
-		store.GetData()["SkipLocalTwoFA"] = true
+	if skipper, ok := source.Cfg.(LocalTwoFASkipper); !ok || !skipper.IsSkipLocalTwoFA() {
+		if err := validateTOTP(req, u); err != nil {
+			return nil, err
+		}
 	}
 
 	log.Trace("Basic Authorization: Logged in user %-v", u)
 
 	return u, nil
 }
+
+func validateTOTP(req *http.Request, u *user_model.User) error {
+	twofa, err := auth_model.GetTwoFactorByUID(u.ID)
+	if err != nil {
+		if auth_model.IsErrTwoFactorNotEnrolled(err) {
+			// No 2FA enrollment for this user
+			return nil
+		}
+		return err
+	}
+	if ok, err := twofa.ValidateTOTP(req.Header.Get("X-Gitea-OTP")); err != nil {
+		return err
+	} else if !ok {
+		return util.NewInvalidArgumentErrorf("invalid provided OTP")
+	}
+	return nil
+}

services/auth/middleware.go

@@ -7,7 +7,6 @@ import (
 	"net/http"
 	"strings"
 
-	"code.gitea.io/gitea/models/auth"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/log"
@@ -216,31 +215,6 @@ func VerifyAuthWithOptionsAPI(options *VerifyOptions) func(ctx *context.APIConte
 				})
 				return
 			}
-			if ctx.IsSigned && ctx.IsBasicAuth {
-				if skip, ok := ctx.Data["SkipLocalTwoFA"]; ok && skip.(bool) {
-					return // Skip 2FA
-				}
-				twofa, err := auth.GetTwoFactorByUID(ctx.Doer.ID)
-				if err != nil {
-					if auth.IsErrTwoFactorNotEnrolled(err) {
-						return // No 2FA enrollment for this user
-					}
-					ctx.InternalServerError(err)
-					return
-				}
-				otpHeader := ctx.Req.Header.Get("X-Gitea-OTP")
-				ok, err := twofa.ValidateTOTP(otpHeader)
-				if err != nil {
-					ctx.InternalServerError(err)
-					return
-				}
-				if !ok {
-					ctx.JSON(http.StatusForbidden, map[string]string{
-						"message": "Only signed in user is allowed to call APIs.",
-					})
-					return
-				}
-			}
 		}
 
 		if options.AdminRequired {

tests/integration/api_twofa_test.go

@@ -0,0 +1,54 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/http"
+	"testing"
+	"time"
+
+	auth_model "code.gitea.io/gitea/models/auth"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/tests"
+
+	"github.com/pquerna/otp/totp"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestAPITwoFactor(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 16})
+
+	req := NewRequestf(t, "GET", "/api/v1/user")
+	req = AddBasicAuthHeader(req, user.Name)
+	MakeRequest(t, req, http.StatusOK)
+
+	otpKey, err := totp.Generate(totp.GenerateOpts{
+		SecretSize:  40,
+		Issuer:      "gitea-test",
+		AccountName: user.Name,
+	})
+	assert.NoError(t, err)
+
+	tfa := &auth_model.TwoFactor{
+		UID: user.ID,
+	}
+	assert.NoError(t, tfa.SetSecret(otpKey.Secret()))
+
+	assert.NoError(t, auth_model.NewTwoFactor(tfa))
+
+	req = NewRequestf(t, "GET", "/api/v1/user")
+	req = AddBasicAuthHeader(req, user.Name)
+	MakeRequest(t, req, http.StatusUnauthorized)
+
+	passcode, err := totp.GenerateCode(otpKey.Secret(), time.Now())
+	assert.NoError(t, err)
+
+	req = NewRequestf(t, "GET", "/api/v1/user")
+	req = AddBasicAuthHeader(req, user.Name)
+	req.Header.Set("X-Gitea-OTP", passcode)
+	MakeRequest(t, req, http.StatusOK)
+}