From f2ee86472d8e6e108efb8c518508d9199428fee2 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Fri, 30 Dec 2022 23:20:26 +1100 Subject: [PATCH] revert: 2fa skip --- .../handlers/handler_register_webauthn.go | 6 -- internal/middlewares/identity_verification.go | 16 ----- .../middlewares/identity_verification_test.go | 63 ------------------- internal/middlewares/types.go | 10 --- 4 files changed, 95 deletions(-) diff --git a/internal/handlers/handler_register_webauthn.go b/internal/handlers/handler_register_webauthn.go index ca13777d2..bc0f001b4 100644 --- a/internal/handlers/handler_register_webauthn.go +++ b/internal/handlers/handler_register_webauthn.go @@ -17,9 +17,6 @@ import ( // WebauthnIdentityStart the handler for initiating the identity validation. var WebauthnIdentityStart = middlewares.IdentityVerificationStart( middlewares.IdentityVerificationStartArgs{ - IdentityVerificationCommonArgs: middlewares.IdentityVerificationCommonArgs{ - SkipIfAuthLevelTwoFactor: true, - }, MailTitle: "Register your key", MailButtonContent: "Register", TargetEndpoint: "/webauthn/register", @@ -30,9 +27,6 @@ var WebauthnIdentityStart = middlewares.IdentityVerificationStart( // WebauthnIdentityFinish the handler for finishing the identity validation. var WebauthnIdentityFinish = middlewares.IdentityVerificationFinish( middlewares.IdentityVerificationFinishArgs{ - IdentityVerificationCommonArgs: middlewares.IdentityVerificationCommonArgs{ - SkipIfAuthLevelTwoFactor: true, - }, ActionClaim: ActionWebauthnRegistration, IsTokenUserValidFunc: isTokenUserValidFor2FARegistration, }, SecondFactorWebauthnAttestationGET) diff --git a/internal/middlewares/identity_verification.go b/internal/middlewares/identity_verification.go index 79cc167d4..825918e72 100644 --- a/internal/middlewares/identity_verification.go +++ b/internal/middlewares/identity_verification.go @@ -10,16 +10,10 @@ import ( "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" - "github.com/authelia/authelia/v4/internal/authentication" "github.com/authelia/authelia/v4/internal/model" "github.com/authelia/authelia/v4/internal/templates" ) -// Return true if skip enabled at TwoFactor auth level and user's auth level is 2FA, false otherwise. -func shouldSkipIdentityVerification(args IdentityVerificationCommonArgs, ctx *AutheliaCtx) bool { - return args.SkipIfAuthLevelTwoFactor && ctx.GetSession().AuthenticationLevel >= authentication.TwoFactor -} - // IdentityVerificationStart the handler for initiating the identity validation process. func IdentityVerificationStart(args IdentityVerificationStartArgs, delayFunc TimingAttackDelayFunc) RequestHandler { if args.IdentityRetrieverFunc == nil { @@ -27,11 +21,6 @@ func IdentityVerificationStart(args IdentityVerificationStartArgs, delayFunc Tim } return func(ctx *AutheliaCtx) { - if shouldSkipIdentityVerification(args.IdentityVerificationCommonArgs, ctx) { - ctx.ReplyOK() - return - } - requestTime := time.Now() success := false @@ -155,11 +144,6 @@ func identityVerificationValidateToken(ctx *AutheliaCtx) (*jwt.Token, error) { // IdentityVerificationFinish the middleware for finishing the identity validation process. func IdentityVerificationFinish(args IdentityVerificationFinishArgs, next func(ctx *AutheliaCtx, username string)) RequestHandler { return func(ctx *AutheliaCtx) { - if shouldSkipIdentityVerification(args.IdentityVerificationCommonArgs, ctx) { - next(ctx, "") - return - } - token, err := identityVerificationValidateToken(ctx) if token == nil || err != nil { return diff --git a/internal/middlewares/identity_verification_test.go b/internal/middlewares/identity_verification_test.go index f5a5cf3b7..bb81141fa 100644 --- a/internal/middlewares/identity_verification_test.go +++ b/internal/middlewares/identity_verification_test.go @@ -12,7 +12,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" - "github.com/authelia/authelia/v4/internal/authentication" "github.com/authelia/authelia/v4/internal/middlewares" "github.com/authelia/authelia/v4/internal/mocks" "github.com/authelia/authelia/v4/internal/model" @@ -38,38 +37,6 @@ func defaultRetriever(ctx *middlewares.AutheliaCtx) (*session.Identity, error) { }, nil } -func TestShouldSkipStartIdentityVerificationIf2FASkipEnabled(t *testing.T) { - testCases := []bool{true, false} - for _, testCaseSkipEnabled := range testCases { - t.Run(fmt.Sprintf("SkipIfAuthLevelTwoFactor=%t", testCaseSkipEnabled), func(t *testing.T) { - mock := mocks.NewMockAutheliaCtx(t) - defer mock.Close() - - mock.Ctx.Request.Header.Add("X-Forwarded-Proto", "http") - mock.Ctx.Request.Header.Add("X-Forwarded-Host", "host") - - if testCaseSkipEnabled == false { - mock.StorageMock.EXPECT(). - SaveIdentityVerification(mock.Ctx, gomock.Any()). - Return(nil) - mock.NotifierMock.EXPECT(). - Send(gomock.Eq(mail.Address{Address: "john@example.com"}), gomock.Eq("Title"), gomock.Any(), gomock.Any()). - Return(nil) - } - - userSession := mock.Ctx.GetSession() - userSession.AuthenticationLevel = authentication.TwoFactor - assert.NoError(t, mock.Ctx.SaveSession(userSession)) - - args := newArgs(defaultRetriever) - args.IdentityVerificationCommonArgs.SkipIfAuthLevelTwoFactor = testCaseSkipEnabled - middlewares.IdentityVerificationStart(args, nil)(mock.Ctx) - - assert.Equal(t, 200, mock.Ctx.Response.StatusCode()) - }) - } -} - func TestShouldFailStartingProcessIfUserHasNoEmailAddress(t *testing.T) { mock := mocks.NewMockAutheliaCtx(t) defer mock.Close() @@ -307,36 +274,6 @@ func (s *IdentityVerificationFinishProcess) TestShouldReturn200OnFinishComplete( assert.Equal(s.T(), 200, s.mock.Ctx.Response.StatusCode()) } -func (s *IdentityVerificationFinishProcess) TestShouldSkipIf2FASkipEnabled() { - testCases := []bool{true, false} - for _, testCaseSkipEnabled := range testCases { - s.Run(fmt.Sprintf("SkipIfAuthLevelTwoFactor=%t", testCaseSkipEnabled), func() { - token, verification := createToken(s.mock, "john", "EXP_ACTION", - time.Now().Add(1*time.Minute)) - s.mock.Ctx.Request.SetBodyString(fmt.Sprintf("{\"token\":\"%s\"}", token)) - - if testCaseSkipEnabled == false { - s.mock.StorageMock.EXPECT(). - FindIdentityVerification(s.mock.Ctx, gomock.Eq(verification.JTI.String())). - Return(true, nil) - s.mock.StorageMock.EXPECT(). - ConsumeIdentityVerification(s.mock.Ctx, gomock.Eq(verification.JTI.String()), gomock.Eq(model.NewNullIP(s.mock.Ctx.RemoteIP()))). - Return(nil) - } - - userSession := s.mock.Ctx.GetSession() - userSession.AuthenticationLevel = authentication.TwoFactor - assert.NoError(s.T(), s.mock.Ctx.SaveSession(userSession)) - - args := newFinishArgs() - args.IdentityVerificationCommonArgs.SkipIfAuthLevelTwoFactor = testCaseSkipEnabled - middlewares.IdentityVerificationFinish(args, next)(s.mock.Ctx) - - assert.Equal(s.T(), 200, s.mock.Ctx.Response.StatusCode()) - }) - } -} - func TestRunIdentityVerificationFinish(t *testing.T) { s := new(IdentityVerificationFinishProcess) suite.Run(t, s) diff --git a/internal/middlewares/types.go b/internal/middlewares/types.go index 906c7e3ae..f26e57fc7 100644 --- a/internal/middlewares/types.go +++ b/internal/middlewares/types.go @@ -70,17 +70,9 @@ type BridgeBuilder struct { // Basic represents a middleware applied to a fasthttp.RequestHandler. type Basic func(next fasthttp.RequestHandler) (handler fasthttp.RequestHandler) -// IdentityVerificationCommonArgs contains shared options for both verification start and finish steps. -type IdentityVerificationCommonArgs struct { - // If true, skip identity verification if the user's AuthenticationLevel is TwoFactor. Otherwise, always perform identity verification. - SkipIfAuthLevelTwoFactor bool -} - // IdentityVerificationStartArgs represent the arguments used to customize the starting phase // of the identity verification process. type IdentityVerificationStartArgs struct { - IdentityVerificationCommonArgs - // Email template needs a subject, a title and the content of the button. MailTitle string MailButtonContent string @@ -102,8 +94,6 @@ type IdentityVerificationStartArgs struct { // IdentityVerificationFinishArgs represent the arguments used to customize the finishing phase // of the identity verification process. type IdentityVerificationFinishArgs struct { - IdentityVerificationCommonArgs - // The action claim that should be in the token to consider the action legitimate. ActionClaim string