From be802cfc7b5cf7d8c85bb7053dd652efa992ed00 Mon Sep 17 00:00:00 2001 From: Clement Michaud Date: Sat, 30 Nov 2019 15:33:45 +0100 Subject: [PATCH] Fix unit tests. --- internal/handlers/handler_firstfactor.go | 16 +- internal/handlers/handler_firstfactor_test.go | 31 ++- internal/handlers/handler_verify_test.go | 184 +++++++++++++----- internal/mocks/mock_authelia_ctx.go | 3 + internal/regulation/regulator_test.go | 59 +++--- 5 files changed, 193 insertions(+), 100 deletions(-) diff --git a/internal/handlers/handler_firstfactor.go b/internal/handlers/handler_firstfactor.go index 0f9c3ed72..fff587d47 100644 --- a/internal/handlers/handler_firstfactor.go +++ b/internal/handlers/handler_firstfactor.go @@ -42,14 +42,6 @@ func FirstFactorPost(ctx *middlewares.AutheliaCtx) { return } - ctx.Logger.Debugf("Mark authentication attempt made by user %s", bodyJSON.Username) - err = ctx.Providers.Regulator.Mark(bodyJSON.Username, false) - - if err != nil { - ctx.Error(fmt.Errorf("Unable to mark authentication: %s", err), authenticationFailedMessage) - return - } - if !userPasswordOk { ctx.ReplyError(fmt.Errorf("Credentials are wrong for user %s", bodyJSON.Username), authenticationFailedMessage) return @@ -57,6 +49,14 @@ func FirstFactorPost(ctx *middlewares.AutheliaCtx) { ctx.Logger.Debugf("Credentials validation of user %s is ok", bodyJSON.Username) + ctx.Logger.Debugf("Mark authentication attempt made by user %s", bodyJSON.Username) + err = ctx.Providers.Regulator.Mark(bodyJSON.Username, true) + + if err != nil { + ctx.Error(fmt.Errorf("Unable to mark authentication: %s", err), authenticationFailedMessage) + return + } + // Reset all values from previous session before regenerating the cookie. err = ctx.SaveSession(session.NewDefaultUserSession()) diff --git a/internal/handlers/handler_firstfactor_test.go b/internal/handlers/handler_firstfactor_test.go index 241c362a0..b9c61cc9e 100644 --- a/internal/handlers/handler_firstfactor_test.go +++ b/internal/handlers/handler_firstfactor_test.go @@ -3,7 +3,6 @@ package handlers import ( "fmt" "testing" - "time" "github.com/clems4ever/authelia/internal/mocks" "github.com/clems4ever/authelia/internal/models" @@ -61,6 +60,14 @@ func (s *FirstFactorSuite) TestShouldFailIfUserProviderCheckPasswordFail() { CheckUserPassword(gomock.Eq("test"), gomock.Eq("hello")). Return(false, fmt.Errorf("Failed")) + s.mock.StorageProviderMock. + EXPECT(). + AppendAuthenticationLog(gomock.Eq(models.AuthenticationAttempt{ + Username: "test", + Successful: false, + Time: s.mock.Clock.Now(), + })) + s.mock.Ctx.Request.SetBodyString(`{ "username": "test", "password": "hello", @@ -73,9 +80,6 @@ func (s *FirstFactorSuite) TestShouldFailIfUserProviderCheckPasswordFail() { } func (s *FirstFactorSuite) TestShouldCheckAuthenticationIsMarkedWhenInvalidCredentials() { - t, _ := time.Parse("2006-Jan-02", "2013-Feb-03") - s.mock.Clock.Set(t) - s.mock.UserProviderMock. EXPECT(). CheckUserPassword(gomock.Eq("test"), gomock.Eq("hello")). @@ -86,7 +90,7 @@ func (s *FirstFactorSuite) TestShouldCheckAuthenticationIsMarkedWhenInvalidCrede AppendAuthenticationLog(gomock.Eq(models.AuthenticationAttempt{ Username: "test", Successful: false, - Time: t, + Time: s.mock.Clock.Now(), })) s.mock.Ctx.Request.SetBodyString(`{ @@ -104,16 +108,16 @@ func (s *FirstFactorSuite) TestShouldFailIfUserProviderGetDetailsFail() { CheckUserPassword(gomock.Eq("test"), gomock.Eq("hello")). Return(true, nil) - s.mock.UserProviderMock. - EXPECT(). - GetDetails(gomock.Eq("test")). - Return(nil, fmt.Errorf("Failed")) - s.mock.StorageProviderMock. EXPECT(). AppendAuthenticationLog(gomock.Any()). Return(nil) + s.mock.UserProviderMock. + EXPECT(). + GetDetails(gomock.Eq("test")). + Return(nil, fmt.Errorf("Failed")) + s.mock.Ctx.Request.SetBodyString(`{ "username": "test", "password": "hello", @@ -125,17 +129,12 @@ func (s *FirstFactorSuite) TestShouldFailIfUserProviderGetDetailsFail() { s.mock.Assert200KO(s.T(), "Authentication failed. Check your credentials.") } -func (s *FirstFactorSuite) TestShouldFailIfAuthenticationLoggingFail() { +func (s *FirstFactorSuite) TestShouldFailIfAuthenticationMarkFail() { s.mock.UserProviderMock. EXPECT(). CheckUserPassword(gomock.Eq("test"), gomock.Eq("hello")). Return(true, nil) - s.mock.UserProviderMock. - EXPECT(). - GetDetails(gomock.Eq("test")). - Return(nil, nil) - s.mock.StorageProviderMock. EXPECT(). AppendAuthenticationLog(gomock.Any()). diff --git a/internal/handlers/handler_verify_test.go b/internal/handlers/handler_verify_test.go index f478cf91b..aca2d039a 100644 --- a/internal/handlers/handler_verify_test.go +++ b/internal/handlers/handler_verify_test.go @@ -12,6 +12,7 @@ import ( "github.com/clems4ever/authelia/internal/mocks" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" ) // Test getOriginalURL @@ -172,54 +173,143 @@ func (tc TestCase) String() string { return fmt.Sprintf("url=%s, auth=%s, exp_status=%d", tc.URL, tc.Authorization, tc.ExpectedStatusCode) } +type BasicAuthorizationSuite struct { + suite.Suite +} + +func NewBasicAuthorizationSuite() *BasicAuthorizationSuite { + return &BasicAuthorizationSuite{} +} + +func (s *BasicAuthorizationSuite) TestShouldNotBeAbleToParseBasicAuth() { + mock := mocks.NewMockAutheliaCtx(s.T()) + defer mock.Close() + + mock.Ctx.Request.Header.Set("Proxy-Authorization", "Basic am9objpaaaaaaaaaaaaaaaa") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://test.example.com") + + VerifyGet(mock.Ctx) + + assert.Equal(s.T(), 401, mock.Ctx.Response.StatusCode()) +} + +func (s *BasicAuthorizationSuite) TestShouldApplyDefaultPolicy() { + mock := mocks.NewMockAutheliaCtx(s.T()) + defer mock.Close() + + mock.Ctx.Request.Header.Set("Proxy-Authorization", "Basic am9objpwYXNzd29yZA==") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://test.example.com") + + mock.UserProviderMock.EXPECT(). + CheckUserPassword(gomock.Eq("john"), gomock.Eq("password")). + Return(true, nil) + + mock.UserProviderMock.EXPECT(). + GetDetails(gomock.Eq("john")). + Return(&authentication.UserDetails{ + Emails: []string{"john@example.com"}, + Groups: []string{"dev", "admin"}, + }, nil) + + VerifyGet(mock.Ctx) + + assert.Equal(s.T(), 403, mock.Ctx.Response.StatusCode()) +} + +func (s *BasicAuthorizationSuite) TestShouldApplyPolicyOfBypassDomain() { + mock := mocks.NewMockAutheliaCtx(s.T()) + defer mock.Close() + + mock.Ctx.Request.Header.Set("Proxy-Authorization", "Basic am9objpwYXNzd29yZA==") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://bypass.example.com") + + mock.UserProviderMock.EXPECT(). + CheckUserPassword(gomock.Eq("john"), gomock.Eq("password")). + Return(true, nil) + + mock.UserProviderMock.EXPECT(). + GetDetails(gomock.Eq("john")). + Return(&authentication.UserDetails{ + Emails: []string{"john@example.com"}, + Groups: []string{"dev", "admin"}, + }, nil) + + VerifyGet(mock.Ctx) + + assert.Equal(s.T(), 200, mock.Ctx.Response.StatusCode()) +} + +func (s *BasicAuthorizationSuite) TestShouldApplyPolicyOfOneFactorDomain() { + mock := mocks.NewMockAutheliaCtx(s.T()) + defer mock.Close() + + mock.Ctx.Request.Header.Set("Proxy-Authorization", "Basic am9objpwYXNzd29yZA==") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://one-factor.example.com") + + mock.UserProviderMock.EXPECT(). + CheckUserPassword(gomock.Eq("john"), gomock.Eq("password")). + Return(true, nil) + + mock.UserProviderMock.EXPECT(). + GetDetails(gomock.Eq("john")). + Return(&authentication.UserDetails{ + Emails: []string{"john@example.com"}, + Groups: []string{"dev", "admin"}, + }, nil) + + VerifyGet(mock.Ctx) + + assert.Equal(s.T(), 200, mock.Ctx.Response.StatusCode()) +} + +func (s *BasicAuthorizationSuite) TestShouldApplyPolicyOfTwoFactorDomain() { + mock := mocks.NewMockAutheliaCtx(s.T()) + defer mock.Close() + + mock.Ctx.Request.Header.Set("Proxy-Authorization", "Basic am9objpwYXNzd29yZA==") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com") + + mock.UserProviderMock.EXPECT(). + CheckUserPassword(gomock.Eq("john"), gomock.Eq("password")). + Return(true, nil) + + mock.UserProviderMock.EXPECT(). + GetDetails(gomock.Eq("john")). + Return(&authentication.UserDetails{ + Emails: []string{"john@example.com"}, + Groups: []string{"dev", "admin"}, + }, nil) + + VerifyGet(mock.Ctx) + + assert.Equal(s.T(), 401, mock.Ctx.Response.StatusCode()) +} + +func (s *BasicAuthorizationSuite) TestShouldApplyPolicyOfDenyDomain() { + mock := mocks.NewMockAutheliaCtx(s.T()) + defer mock.Close() + + mock.Ctx.Request.Header.Set("Proxy-Authorization", "Basic am9objpwYXNzd29yZA==") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://deny.example.com") + + mock.UserProviderMock.EXPECT(). + CheckUserPassword(gomock.Eq("john"), gomock.Eq("password")). + Return(true, nil) + + mock.UserProviderMock.EXPECT(). + GetDetails(gomock.Eq("john")). + Return(&authentication.UserDetails{ + Emails: []string{"john@example.com"}, + Groups: []string{"dev", "admin"}, + }, nil) + + VerifyGet(mock.Ctx) + + assert.Equal(s.T(), 403, mock.Ctx.Response.StatusCode()) +} + func TestShouldVerifyAuthorizationsUsingBasicAuth(t *testing.T) { - testCases := []TestCase{ - // Authorization has bad format. - TestCase{"https://bypass.example.com", "Basic am9objpaaaaaaaaaaaaaaaa", 401}, - - // Correct Authorization - TestCase{"https://test.example.com", "Basic am9objpwYXNzd29yZA==", 403}, - TestCase{"https://bypass.example.com", "Basic am9objpwYXNzd29yZA==", 200}, - TestCase{"https://one-factor.example.com", "Basic am9objpwYXNzd29yZA==", 200}, - TestCase{"https://two-factor.example.com", "Basic am9objpwYXNzd29yZA==", 401}, - TestCase{"https://deny.example.com", "Basic am9objpwYXNzd29yZA==", 403}, - } - - for _, testCase := range testCases { - testCase := testCase - t.Run(testCase.String(), func(t *testing.T) { - mock := mocks.NewMockAutheliaCtx(t) - defer mock.Close() - - mock.UserProviderMock.EXPECT(). - CheckUserPassword(gomock.Eq("john"), gomock.Eq("password")). - Return(true, nil) - - details := authentication.UserDetails{ - Emails: []string{"john@example.com"}, - Groups: []string{"dev", "admin"}, - } - mock.UserProviderMock.EXPECT(). - GetDetails(gomock.Eq("john")). - Return(&details, nil) - - mock.Ctx.Request.Header.Set("Proxy-Authorization", testCase.Authorization) - mock.Ctx.Request.Header.Set("X-Original-URL", testCase.URL) - - VerifyGet(mock.Ctx) - expStatus, actualStatus := testCase.ExpectedStatusCode, mock.Ctx.Response.StatusCode() - assert.Equal(t, expStatus, actualStatus, "URL=%s -> StatusCode=%d != ExpectedStatusCode=%d", - testCase.URL, actualStatus, expStatus) - - if testCase.ExpectedStatusCode == 200 { - assert.Equal(t, []byte("john"), mock.Ctx.Response.Header.Peek("Remote-User")) - assert.Equal(t, []byte("dev,admin"), mock.Ctx.Response.Header.Peek("Remote-Groups")) - } else { - assert.Equal(t, []byte(nil), mock.Ctx.Response.Header.Peek("Remote-User")) - assert.Equal(t, []byte(nil), mock.Ctx.Response.Header.Peek("Remote-Groups")) - } - }) - } + suite.Run(t, NewBasicAuthorizationSuite()) } func TestShouldVerifyWrongCredentialsInBasicAuth(t *testing.T) { diff --git a/internal/mocks/mock_authelia_ctx.go b/internal/mocks/mock_authelia_ctx.go index 148794d79..870343e2b 100644 --- a/internal/mocks/mock_authelia_ctx.go +++ b/internal/mocks/mock_authelia_ctx.go @@ -62,6 +62,9 @@ func NewMockAutheliaCtx(t *testing.T) *MockAutheliaCtx { mockAuthelia := new(MockAutheliaCtx) mockAuthelia.Clock = TestingClock{} + datetime, _ := time.Parse("2006-Jan-02", "2013-Feb-03") + mockAuthelia.Clock.Set(datetime) + configuration := schema.Configuration{ AccessControl: new(schema.AccessControlConfiguration), } diff --git a/internal/regulation/regulator_test.go b/internal/regulation/regulator_test.go index ae3f3701e..00b8a3ed2 100644 --- a/internal/regulation/regulator_test.go +++ b/internal/regulation/regulator_test.go @@ -5,6 +5,7 @@ import ( "time" "github.com/clems4ever/authelia/internal/configuration/schema" + "github.com/clems4ever/authelia/internal/mocks" "github.com/clems4ever/authelia/internal/models" "github.com/clems4ever/authelia/internal/regulation" "github.com/clems4ever/authelia/internal/storage" @@ -19,7 +20,7 @@ type RegulatorSuite struct { ctrl *gomock.Controller storageMock *storage.MockProvider configuration schema.RegulationConfiguration - now time.Time + clock mocks.TestingClock } func (s *RegulatorSuite) SetupTest() { @@ -31,7 +32,7 @@ func (s *RegulatorSuite) SetupTest() { BanTime: 180, FindTime: 30, } - s.now = time.Now() + s.clock.Set(time.Now()) } func (s *RegulatorSuite) TearDownTest() { @@ -43,7 +44,7 @@ func (s *RegulatorSuite) TestShouldNotThrowWhenUserIsLegitimate() { models.AuthenticationAttempt{ Username: "john", Successful: true, - Time: s.now.Add(-4 * time.Minute), + Time: s.clock.Now().Add(-4 * time.Minute), }, } @@ -51,7 +52,7 @@ func (s *RegulatorSuite) TestShouldNotThrowWhenUserIsLegitimate() { LoadLatestAuthenticationLogs(gomock.Eq("john"), gomock.Any()). Return(attempts, nil) - regulator := regulation.NewRegulator(&s.configuration, s.storageMock) + regulator := regulation.NewRegulator(&s.configuration, s.storageMock, &s.clock) _, err := regulator.Regulate("john") assert.NoError(s.T(), err) @@ -64,17 +65,17 @@ func (s *RegulatorSuite) TestShouldNotThrowWhenFailedAuthenticationNotInFindTime models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-1 * time.Second), + Time: s.clock.Now().Add(-1 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-90 * time.Second), + Time: s.clock.Now().Add(-90 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-180 * time.Second), + Time: s.clock.Now().Add(-180 * time.Second), }, } @@ -82,7 +83,7 @@ func (s *RegulatorSuite) TestShouldNotThrowWhenFailedAuthenticationNotInFindTime LoadLatestAuthenticationLogs(gomock.Eq("john"), gomock.Any()). Return(attemptsInDB, nil) - regulator := regulation.NewRegulator(&s.configuration, s.storageMock) + regulator := regulation.NewRegulator(&s.configuration, s.storageMock, &s.clock) _, err := regulator.Regulate("john") assert.NoError(s.T(), err) @@ -95,22 +96,22 @@ func (s *RegulatorSuite) TestShouldBanUserIfLatestAttemptsAreWithinFinTime() { models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-1 * time.Second), + Time: s.clock.Now().Add(-1 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-4 * time.Second), + Time: s.clock.Now().Add(-4 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-6 * time.Second), + Time: s.clock.Now().Add(-6 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-180 * time.Second), + Time: s.clock.Now().Add(-180 * time.Second), }, } @@ -118,7 +119,7 @@ func (s *RegulatorSuite) TestShouldBanUserIfLatestAttemptsAreWithinFinTime() { LoadLatestAuthenticationLogs(gomock.Eq("john"), gomock.Any()). Return(attemptsInDB, nil) - regulator := regulation.NewRegulator(&s.configuration, s.storageMock) + regulator := regulation.NewRegulator(&s.configuration, s.storageMock, &s.clock) _, err := regulator.Regulate("john") assert.Equal(s.T(), regulation.ErrUserIsBanned, err) @@ -133,17 +134,17 @@ func (s *RegulatorSuite) TestShouldCheckUserIsStillBanned() { models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-31 * time.Second), + Time: s.clock.Now().Add(-31 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-34 * time.Second), + Time: s.clock.Now().Add(-34 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-36 * time.Second), + Time: s.clock.Now().Add(-36 * time.Second), }, } @@ -151,7 +152,7 @@ func (s *RegulatorSuite) TestShouldCheckUserIsStillBanned() { LoadLatestAuthenticationLogs(gomock.Eq("john"), gomock.Any()). Return(attemptsInDB, nil) - regulator := regulation.NewRegulator(&s.configuration, s.storageMock) + regulator := regulation.NewRegulator(&s.configuration, s.storageMock, &s.clock) _, err := regulator.Regulate("john") assert.Equal(s.T(), regulation.ErrUserIsBanned, err) @@ -162,12 +163,12 @@ func (s *RegulatorSuite) TestShouldCheckUserIsNotYetBanned() { models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-34 * time.Second), + Time: s.clock.Now().Add(-34 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-36 * time.Second), + Time: s.clock.Now().Add(-36 * time.Second), }, } @@ -175,7 +176,7 @@ func (s *RegulatorSuite) TestShouldCheckUserIsNotYetBanned() { LoadLatestAuthenticationLogs(gomock.Eq("john"), gomock.Any()). Return(attemptsInDB, nil) - regulator := regulation.NewRegulator(&s.configuration, s.storageMock) + regulator := regulation.NewRegulator(&s.configuration, s.storageMock, &s.clock) _, err := regulator.Regulate("john") assert.NoError(s.T(), err) @@ -186,7 +187,7 @@ func (s *RegulatorSuite) TestShouldCheckUserWasAboutToBeBanned() { models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-14 * time.Second), + Time: s.clock.Now().Add(-14 * time.Second), }, // more than 30 seconds elapsed between this auth and the preceding one. // In that case we don't need to regulate the user even though the number @@ -194,12 +195,12 @@ func (s *RegulatorSuite) TestShouldCheckUserWasAboutToBeBanned() { models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-94 * time.Second), + Time: s.clock.Now().Add(-94 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-96 * time.Second), + Time: s.clock.Now().Add(-96 * time.Second), }, } @@ -207,7 +208,7 @@ func (s *RegulatorSuite) TestShouldCheckUserWasAboutToBeBanned() { LoadLatestAuthenticationLogs(gomock.Eq("john"), gomock.Any()). Return(attemptsInDB, nil) - regulator := regulation.NewRegulator(&s.configuration, s.storageMock) + regulator := regulation.NewRegulator(&s.configuration, s.storageMock, &s.clock) _, err := regulator.Regulate("john") assert.NoError(s.T(), err) @@ -218,24 +219,24 @@ func (s *RegulatorSuite) TestShouldCheckRegulationHasBeenResetOnSuccessfulAttemp models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-90 * time.Second), + Time: s.clock.Now().Add(-90 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: true, - Time: s.now.Add(-93 * time.Second), + Time: s.clock.Now().Add(-93 * time.Second), }, // The user was almost banned but he did a successful attempt. Therefore, even if the next // failure happens withing FindTime, he should not be banned. models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-94 * time.Second), + Time: s.clock.Now().Add(-94 * time.Second), }, models.AuthenticationAttempt{ Username: "john", Successful: false, - Time: s.now.Add(-96 * time.Second), + Time: s.clock.Now().Add(-96 * time.Second), }, } @@ -243,7 +244,7 @@ func (s *RegulatorSuite) TestShouldCheckRegulationHasBeenResetOnSuccessfulAttemp LoadLatestAuthenticationLogs(gomock.Eq("john"), gomock.Any()). Return(attemptsInDB, nil) - regulator := regulation.NewRegulator(&s.configuration, s.storageMock) + regulator := regulation.NewRegulator(&s.configuration, s.storageMock, &s.clock) _, err := regulator.Regulate("john") assert.NoError(s.T(), err)