From da012ab2d660e742f03e321a38fa5569fd93cd3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Nu=C3=B1ez?= <10672208+mind-ar@users.noreply.github.com> Date: Mon, 4 Jul 2022 20:58:35 -0300 Subject: [PATCH] fix(handlers): fix redirect with timed out sessions on rules with bypass policy (#3599) This change replaced a returned error with a warning when the idle timeout was exceeded. Fixes #3587 --- internal/handlers/handler_verify.go | 12 +++--- internal/handlers/handler_verify_test.go | 50 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/internal/handlers/handler_verify.go b/internal/handlers/handler_verify.go index 5d4ea3729..f16ff8acc 100644 --- a/internal/handlers/handler_verify.go +++ b/internal/handlers/handler_verify.go @@ -171,7 +171,9 @@ func verifySessionCookie(ctx *middlewares.AutheliaCtx, targetURL *url.URL, userS return "", "", nil, nil, authentication.NotAuthenticated, fmt.Errorf("unable to destroy user session after long inactivity: %s", err) } - return userSession.Username, userSession.DisplayName, userSession.Groups, userSession.Emails, authentication.NotAuthenticated, fmt.Errorf("User %s has been inactive for too long", userSession.Username) + ctx.Logger.Warnf("User %s has been inactive for too long", userSession.Username) + + return "", "", nil, nil, authentication.NotAuthenticated, nil } } @@ -260,8 +262,8 @@ func handleUnauthorized(ctx *middlewares.AutheliaCtx, targetURL fmt.Stringer, is } } -func updateActivityTimestamp(ctx *middlewares.AutheliaCtx, isBasicAuth bool, username string) error { - if isBasicAuth || username == "" { +func updateActivityTimestamp(ctx *middlewares.AutheliaCtx, isBasicAuth bool) error { + if isBasicAuth { return nil } @@ -477,7 +479,7 @@ func VerifyGET(cfg schema.AuthenticationBackendConfiguration) middlewares.Reques if err != nil { ctx.Logger.Errorf("Error caught when verifying user authorization: %s", err) - if err := updateActivityTimestamp(ctx, isBasicAuth, username); err != nil { + if err := updateActivityTimestamp(ctx, isBasicAuth); err != nil { ctx.Error(fmt.Errorf("unable to update last activity: %s", err), messageOperationFailed) return } @@ -500,7 +502,7 @@ func VerifyGET(cfg schema.AuthenticationBackendConfiguration) middlewares.Reques setForwardedHeaders(&ctx.Response.Header, username, name, groups, emails) } - if err := updateActivityTimestamp(ctx, isBasicAuth, username); err != nil { + if err := updateActivityTimestamp(ctx, isBasicAuth); err != nil { ctx.Error(fmt.Errorf("unable to update last activity: %s", err), messageOperationFailed) } } diff --git a/internal/handlers/handler_verify_test.go b/internal/handlers/handler_verify_test.go index 088197a4c..e7f99a8e7 100644 --- a/internal/handlers/handler_verify_test.go +++ b/internal/handlers/handler_verify_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "github.com/valyala/fasthttp" "github.com/authelia/authelia/v4/internal/authentication" "github.com/authelia/authelia/v4/internal/authorization" @@ -1264,3 +1265,52 @@ func TestGetProfileRefreshSettings(t *testing.T) { assert.Equal(t, true, refresh) assert.Equal(t, time.Duration(0), interval) } + +func TestShouldNotRedirectRequestsForBypassACLWhenInactiveForTooLong(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + clock := mocks.TestingClock{} + clock.Set(time.Now()) + past := clock.Now().Add(-1 * time.Hour) + + mock.Ctx.Configuration.Session.Inactivity = testInactivity + // Reload the session provider since the configuration is indirect. + mock.Ctx.Providers.SessionProvider = session.NewProvider(mock.Ctx.Configuration.Session, nil) + assert.Equal(t, time.Second*10, mock.Ctx.Providers.SessionProvider.Inactivity) + + userSession := mock.Ctx.GetSession() + userSession.Username = testUsername + userSession.AuthenticationLevel = authentication.TwoFactor + userSession.LastActivity = past.Unix() + + err := mock.Ctx.SaveSession(userSession) + require.NoError(t, err) + + // Should respond 200 OK. + mock.Ctx.QueryArgs().Add("rd", "https://login.example.com") + mock.Ctx.Request.Header.Set("X-Forwarded-Method", "GET") + mock.Ctx.Request.Header.Set("Accept", "text/html; charset=utf-8") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://bypass.example.com") + VerifyGET(verifyGetCfg)(mock.Ctx) + assert.Equal(t, fasthttp.StatusOK, mock.Ctx.Response.StatusCode()) + assert.Nil(t, mock.Ctx.Response.Header.Peek("Location")) + + // Should respond 302 Found. + mock.Ctx.QueryArgs().Add("rd", "https://login.example.com") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com") + mock.Ctx.Request.Header.Set("X-Forwarded-Method", "GET") + mock.Ctx.Request.Header.Set("Accept", "text/html; charset=utf-8") + VerifyGET(verifyGetCfg)(mock.Ctx) + assert.Equal(t, fasthttp.StatusFound, mock.Ctx.Response.StatusCode()) + assert.Equal(t, "https://login.example.com/?rd=https%3A%2F%2Ftwo-factor.example.com&rm=GET", string(mock.Ctx.Response.Header.Peek("Location"))) + + // Should respond 401 Unauthorized. + mock.Ctx.QueryArgs().Del("rd") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com") + mock.Ctx.Request.Header.Set("X-Forwarded-Method", "GET") + mock.Ctx.Request.Header.Set("Accept", "text/html; charset=utf-8") + VerifyGET(verifyGetCfg)(mock.Ctx) + assert.Equal(t, fasthttp.StatusUnauthorized, mock.Ctx.Response.StatusCode()) + assert.Nil(t, mock.Ctx.Response.Header.Peek("Location")) +}