diff --git a/internal/handlers/handler_verify.go b/internal/handlers/handler_verify.go index 84753ba0c..0c695c5a5 100644 --- a/internal/handlers/handler_verify.go +++ b/internal/handlers/handler_verify.go @@ -6,7 +6,6 @@ import ( "net" "net/url" "strings" - "time" "github.com/valyala/fasthttp" @@ -195,12 +194,47 @@ func verifyFromSessionCookie(targetURL url.URL, ctx *middlewares.AutheliaCtx) (u return "", nil, authentication.NotAuthenticated, fmt.Errorf("Unable to destroy user session after long inactivity: %s", err) } - return "", nil, authentication.NotAuthenticated, fmt.Errorf("User %s has been inactive for too long", userSession.Username) + return userSession.Username, userSession.Groups, authentication.NotAuthenticated, fmt.Errorf("User %s has been inactive for too long", userSession.Username) } } return userSession.Username, userSession.Groups, userSession.AuthenticationLevel, nil } +func handleUnauthorized(ctx *middlewares.AutheliaCtx, targetURL fmt.Stringer, username string) { + // Kubernetes ingress controller and Traefik use the rd parameter of the verify + // endpoint to provide the URL of the login portal. The target URL of the user + // is computed from X-Fowarded-* headers or X-Original-URL + rd := string(ctx.QueryArgs().Peek("rd")) + if rd != "" { + redirectionURL := fmt.Sprintf("%s?rd=%s", rd, url.QueryEscape(targetURL.String())) + if strings.Contains(redirectionURL, "/%23/") { + ctx.Logger.Warn("Characters /%23/ have been detected in redirection URL. This is not needed anymore, please strip it") + } + ctx.Logger.Infof("Access to %s is not authorized to user %s, redirecting to %s", targetURL.String(), username, redirectionURL) + ctx.Redirect(redirectionURL, 302) + ctx.SetBodyString(fmt.Sprintf("Found. Redirecting to %s", redirectionURL)) + } else { + ctx.Logger.Infof("Access to %s is not authorized to user %s, sending 401 response", targetURL.String(), username) + ctx.ReplyUnauthorized() + } +} + +func updateActivityTimestamp(ctx *middlewares.AutheliaCtx, isBasicAuth bool, username string) error { + if isBasicAuth || username == "" { + return nil + } + + userSession := ctx.GetSession() + // We don't need to update the activity timestamp when user checked keep me logged in. + if userSession.KeepMeLoggedIn { + return nil + } + + // Mark current activity + userSession.LastActivity = ctx.Clock.Now().Unix() + return ctx.SaveSession(userSession) +} + // VerifyGet is the handler verifying if a request is allowed to go through func VerifyGet(ctx *middlewares.AutheliaCtx) { ctx.Logger.Tracef("Headers=%s", ctx.Request.Header.String()) @@ -230,9 +264,9 @@ func VerifyGet(ctx *middlewares.AutheliaCtx) { var authLevel authentication.Level proxyAuthorization := ctx.Request.Header.Peek(AuthorizationHeader) - hasBasicAuth := proxyAuthorization != nil + isBasicAuth := proxyAuthorization != nil - if hasBasicAuth { + if isBasicAuth { username, groups, authLevel, err = verifyBasicAuth(proxyAuthorization, *targetURL, ctx) } else { username, groups, authLevel, err = verifyFromSessionCookie(*targetURL, ctx) @@ -240,7 +274,11 @@ func VerifyGet(ctx *middlewares.AutheliaCtx) { if err != nil { ctx.Logger.Error(fmt.Sprintf("Error caught when verifying user authorization: %s", err)) - ctx.ReplyUnauthorized() + if err := updateActivityTimestamp(ctx, isBasicAuth, username); err != nil { + ctx.Error(fmt.Errorf("Unable to update last activity: %s", err), operationFailedMessage) + return + } + handleUnauthorized(ctx, targetURL, username) return } @@ -248,39 +286,15 @@ func VerifyGet(ctx *middlewares.AutheliaCtx) { groups, ctx.RemoteIP(), authLevel) if authorization == Forbidden { + ctx.Logger.Infof("Access to %s is forbidden to user %s", targetURL.String(), username) ctx.ReplyForbidden() - ctx.Logger.Errorf("Access to %s is forbidden to user %s", targetURL.String(), username) - return } else if authorization == NotAuthorized { - // Kubernetes ingress controller and Traefik use the rd parameter of the verify - // endpoint to provide the URL of the login portal. The target URL of the user - // is computed from X-Fowarded-* headers or X-Original-URL - rd := string(ctx.QueryArgs().Peek("rd")) - if rd != "" { - redirectionURL := fmt.Sprintf("%s?rd=%s", rd, url.QueryEscape(targetURL.String())) - if strings.Contains(redirectionURL, "/%23/") { - ctx.Logger.Warn("Characters /%23/ have been detected in redirection URL. This is not needed anymore, please strip it") - } - ctx.Redirect(redirectionURL, 302) - ctx.SetBodyString(fmt.Sprintf("Found. Redirecting to %s", redirectionURL)) - } else { - ctx.ReplyUnauthorized() - ctx.Logger.Errorf("Access to %s is not authorized to user %s", targetURL.String(), username) - } + handleUnauthorized(ctx, targetURL, username) } else if authorization == Authorized { setForwardedHeaders(&ctx.Response.Header, username, groups) } - // We mark activity of the current user if he comes with a session cookie - if !hasBasicAuth && username != "" { - // Mark current activity - userSession := ctx.GetSession() - userSession.LastActivity = time.Now().Unix() - err = ctx.SaveSession(userSession) - - if err != nil { - ctx.Error(fmt.Errorf("Unable to update last activity: %s", err), operationFailedMessage) - return - } + if err := updateActivityTimestamp(ctx, isBasicAuth, username); err != nil { + ctx.Error(fmt.Errorf("Unable to update last activity: %s", err), operationFailedMessage) } } diff --git a/internal/handlers/handler_verify_test.go b/internal/handlers/handler_verify_test.go index 1e391eb3f..89357a4ce 100644 --- a/internal/handlers/handler_verify_test.go +++ b/internal/handlers/handler_verify_test.go @@ -471,6 +471,7 @@ func TestShouldDestroySessionWhenInactiveForTooLong(t *testing.T) { clock := mocks.TestingClock{} clock.Set(time.Now()) + past := clock.Now().Add(-1 * time.Hour) mock.Ctx.Configuration.Session.Inactivity = "10" // Reload the session provider since the configuration is indirect @@ -480,7 +481,7 @@ func TestShouldDestroySessionWhenInactiveForTooLong(t *testing.T) { userSession := mock.Ctx.GetSession() userSession.Username = "john" userSession.AuthenticationLevel = authentication.TwoFactor - userSession.LastActivity = clock.Now().Add(-1 * time.Hour).Unix() + userSession.LastActivity = past.Unix() mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting. mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com") @@ -491,6 +492,9 @@ func TestShouldDestroySessionWhenInactiveForTooLong(t *testing.T) { newUserSession := mock.Ctx.GetSession() assert.Equal(t, "", newUserSession.Username) assert.Equal(t, authentication.NotAuthenticated, newUserSession.AuthenticationLevel) + + // Check the inactivity timestamp has been updated to current time in the new session. + assert.Equal(t, clock.Now().Unix(), newUserSession.LastActivity) } func TestShouldDestroySessionWhenInactiveForTooLongUsingDurationNotation(t *testing.T) { @@ -533,7 +537,7 @@ func TestShouldKeepSessionWhenUserCheckedRememberMeAndIsInactiveForTooLong(t *te userSession := mock.Ctx.GetSession() userSession.Username = "john" userSession.AuthenticationLevel = authentication.TwoFactor - userSession.LastActivity = clock.Now().Add(-1 * time.Hour).Unix() + userSession.LastActivity = 0 userSession.KeepMeLoggedIn = true mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting. @@ -545,6 +549,9 @@ func TestShouldKeepSessionWhenUserCheckedRememberMeAndIsInactiveForTooLong(t *te newUserSession := mock.Ctx.GetSession() assert.Equal(t, "john", newUserSession.Username) assert.Equal(t, authentication.TwoFactor, newUserSession.AuthenticationLevel) + + // Check the inactivity timestamp is set to 0 in case remember me is checked. + assert.Equal(t, int64(0), newUserSession.LastActivity) } func TestShouldKeepSessionWhenInactivityTimeoutHasNotBeenExceeded(t *testing.T) { @@ -556,10 +563,12 @@ func TestShouldKeepSessionWhenInactivityTimeoutHasNotBeenExceeded(t *testing.T) mock.Ctx.Configuration.Session.Inactivity = "10" + past := clock.Now().Add(-1 * time.Hour) + userSession := mock.Ctx.GetSession() userSession.Username = "john" userSession.AuthenticationLevel = authentication.TwoFactor - userSession.LastActivity = clock.Now().Add(-1 * time.Second).Unix() + userSession.LastActivity = past.Unix() mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting. mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com") @@ -570,6 +579,74 @@ func TestShouldKeepSessionWhenInactivityTimeoutHasNotBeenExceeded(t *testing.T) newUserSession := mock.Ctx.GetSession() assert.Equal(t, "john", newUserSession.Username) assert.Equal(t, authentication.TwoFactor, newUserSession.AuthenticationLevel) + + // Check the inactivity timestamp has been updated to current time in the new session. + assert.Equal(t, clock.Now().Unix(), newUserSession.LastActivity) +} + +// In the case of Traefik and Nginx ingress controller in Kube, the response to an inactive +// session is 302 instead of 401. +func TestShouldRedirectWhenSessionInactiveForTooLongAndRDParamProvided(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + clock := mocks.TestingClock{} + clock.Set(time.Now()) + + mock.Ctx.Configuration.Session.Inactivity = "10" + // Reload the session provider since the configuration is indirect + mock.Ctx.Providers.SessionProvider = session.NewProvider(mock.Ctx.Configuration.Session) + assert.Equal(t, time.Second*10, mock.Ctx.Providers.SessionProvider.Inactivity) + + past := clock.Now().Add(-1 * time.Hour) + + userSession := mock.Ctx.GetSession() + userSession.Username = "john" + userSession.AuthenticationLevel = authentication.TwoFactor + userSession.LastActivity = past.Unix() + mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting. + + mock.Ctx.QueryArgs().Add("rd", "https://login.example.com") + mock.Ctx.Request.Header.Set("X-Original-URL", "https://two-factor.example.com") + + VerifyGet(mock.Ctx) + + assert.Equal(t, "Found. Redirecting to https://login.example.com?rd=https%3A%2F%2Ftwo-factor.example.com", + string(mock.Ctx.Response.Body())) + assert.Equal(t, 302, mock.Ctx.Response.StatusCode()) + + // Check the inactivity timestamp has been updated to current time in the new session. + newUserSession := mock.Ctx.GetSession() + assert.Equal(t, clock.Now().Unix(), newUserSession.LastActivity) +} + +func TestShouldUpdateInactivityTimestampEvenWhenHittingForbiddenResources(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + clock := mocks.TestingClock{} + clock.Set(time.Now()) + + mock.Ctx.Configuration.Session.Inactivity = "10" + + past := clock.Now().Add(-1 * time.Hour) + + userSession := mock.Ctx.GetSession() + userSession.Username = "john" + userSession.AuthenticationLevel = authentication.TwoFactor + userSession.LastActivity = past.Unix() + mock.Ctx.SaveSession(userSession) //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting. + + mock.Ctx.Request.Header.Set("X-Original-URL", "https://deny.example.com") + + VerifyGet(mock.Ctx) + + // The resource if forbidden + assert.Equal(t, 403, mock.Ctx.Response.StatusCode()) + + // Check the inactivity timestamp has been updated to current time in the new session. + newUserSession := mock.Ctx.GetSession() + assert.Equal(t, clock.Now().Unix(), newUserSession.LastActivity) } func TestShouldURLEncodeRedirectionURLParameter(t *testing.T) {