From 00705da102be929dfa41519b030be3bdd8c68472 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Mon, 6 Nov 2023 23:06:21 +0100 Subject: [PATCH] Unify two factor check (#27915) (#27939) Backport of #27915 Fixes #27819 We have support for two factor logins with the normal web login and with basic auth. For basic auth the two factor check was implemented at three different places and you need to know that this check is necessary. This PR moves the check into the basic auth itself. --- modules/context/api.go | 27 --------------- routers/api/v1/api.go | 11 ------ services/auth/basic.go | 24 +++++++++++-- services/auth/middleware.go | 26 -------------- tests/integration/api_twofa_test.go | 54 +++++++++++++++++++++++++++++ 5 files changed, 76 insertions(+), 66 deletions(-) create mode 100644 tests/integration/api_twofa_test.go diff --git a/modules/context/api.go b/modules/context/api.go index f94708d6a3..300eb0ab7a 100644 --- a/modules/context/api.go +++ b/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 { diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 0f7027909d..6d55e8c223 100644 --- a/routers/api/v1/api.go +++ b/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{}, diff --git a/services/auth/basic.go b/services/auth/basic.go index 1352addaaa..5e41730626 100644 --- a/services/auth/basic.go +++ b/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 +} diff --git a/services/auth/middleware.go b/services/auth/middleware.go index d1955a4c90..3e85f0afc1 100644 --- a/services/auth/middleware.go +++ b/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 { diff --git a/tests/integration/api_twofa_test.go b/tests/integration/api_twofa_test.go new file mode 100644 index 0000000000..8ceacf729a --- /dev/null +++ b/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) +}