From 24f5caed9740f71a107af0974436eb70b572512b Mon Sep 17 00:00:00 2001 From: James Elliott Date: Fri, 8 Jul 2022 12:32:43 +1000 Subject: [PATCH] refactor: factorize verify handler (#3662) This factorizes a few sections of the /api/verify handler and improves both the code flow and error output of the section of code. --- internal/handlers/handler_verify.go | 73 +++++++++--------------- internal/handlers/handler_verify_test.go | 64 +++++++++++++++++++++ 2 files changed, 92 insertions(+), 45 deletions(-) diff --git a/internal/handlers/handler_verify.go b/internal/handlers/handler_verify.go index f16ff8acc..266d82350 100644 --- a/internal/handlers/handler_verify.go +++ b/internal/handlers/handler_verify.go @@ -128,24 +128,16 @@ func setForwardedHeaders(headers *fasthttp.ResponseHeader, username, name string } } -// hasUserBeenInactiveTooLong checks whether the user has been inactive for too long. -func hasUserBeenInactiveTooLong(ctx *middlewares.AutheliaCtx) (bool, error) { //nolint:unparam - maxInactivityPeriod := int64(ctx.Providers.SessionProvider.Inactivity.Seconds()) - if maxInactivityPeriod == 0 { - return false, nil +func isSessionInactiveTooLong(ctx *middlewares.AutheliaCtx, userSession *session.UserSession, isUserAnonymous bool) (isInactiveTooLong bool) { + if userSession.KeepMeLoggedIn || isUserAnonymous || int64(ctx.Providers.SessionProvider.Inactivity.Seconds()) == 0 { + return false } - lastActivity := ctx.GetSession().LastActivity - inactivityPeriod := ctx.Clock.Now().Unix() - lastActivity + isInactiveTooLong = time.Unix(userSession.LastActivity, 0).Add(ctx.Providers.SessionProvider.Inactivity).Before(ctx.Clock.Now()) - ctx.Logger.Tracef("Inactivity report: Inactivity=%d, MaxInactivity=%d", - inactivityPeriod, maxInactivityPeriod) + ctx.Logger.Tracef("Inactivity report for user '%s'. Current Time: %d, Last Activity: %d, Maximum Inactivity: %d.", userSession.Username, ctx.Clock.Now().Unix(), userSession.LastActivity, int(ctx.Providers.SessionProvider.Inactivity.Seconds())) - if inactivityPeriod > maxInactivityPeriod { - return true, nil - } - - return false, nil + return isInactiveTooLong } // verifySessionCookie verifies if a user is identified by a cookie. @@ -155,40 +147,30 @@ func verifySessionCookie(ctx *middlewares.AutheliaCtx, targetURL *url.URL, userS isUserAnonymous := userSession.Username == "" if isUserAnonymous && userSession.AuthenticationLevel != authentication.NotAuthenticated { - return "", "", nil, nil, authentication.NotAuthenticated, fmt.Errorf("an anonymous user cannot be authenticated. That might be the sign of a compromise") + return "", "", nil, nil, authentication.NotAuthenticated, fmt.Errorf("an anonymous user cannot be authenticated (this might be the sign of a security compromise)") } - if !userSession.KeepMeLoggedIn && !isUserAnonymous { - inactiveLongEnough, err := hasUserBeenInactiveTooLong(ctx) - if err != nil { - return "", "", nil, nil, authentication.NotAuthenticated, fmt.Errorf("unable to check if user has been inactive for a long time: %s", err) + if isSessionInactiveTooLong(ctx, userSession, isUserAnonymous) { + // Destroy the session a new one will be regenerated on next request. + if err = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx); err != nil { + return "", "", nil, nil, authentication.NotAuthenticated, fmt.Errorf("unable to destroy session for user '%s' after the session has been inactive too long: %w", userSession.Username, err) } - if inactiveLongEnough { - // Destroy the session a new one will be regenerated on next request. - err := ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx) - if err != nil { - return "", "", nil, nil, authentication.NotAuthenticated, fmt.Errorf("unable to destroy user session after long inactivity: %s", err) - } + ctx.Logger.Warnf("Session destroyed for user '%s' after exceeding configured session inactivity and not being marked as remembered", userSession.Username) - ctx.Logger.Warnf("User %s has been inactive for too long", userSession.Username) - - return "", "", nil, nil, authentication.NotAuthenticated, nil - } + return "", "", nil, nil, authentication.NotAuthenticated, nil } - err = verifySessionHasUpToDateProfile(ctx, targetURL, userSession, refreshProfile, refreshProfileInterval) - if err != nil { + if err = verifySessionHasUpToDateProfile(ctx, targetURL, userSession, refreshProfile, refreshProfileInterval); err != nil { if err == authentication.ErrUserNotFound { - err = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx) - if err != nil { - ctx.Logger.Errorf("Unable to destroy user session after provider refresh didn't find the user: %s", err) + if err = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx); err != nil { + ctx.Logger.Errorf("Unable to destroy user session after provider refresh didn't find the user: %v", err) } return userSession.Username, userSession.DisplayName, userSession.Groups, userSession.Emails, authentication.NotAuthenticated, err } - ctx.Logger.Errorf("Error occurred while attempting to update user details from LDAP: %s", err) + ctx.Logger.Errorf("Error occurred while attempting to update user details from LDAP: %v", err) return "", "", nil, nil, authentication.NotAuthenticated, err } @@ -415,31 +397,32 @@ func verifyAuth(ctx *middlewares.AutheliaCtx, targetURL *url.URL, refreshProfile if authValue != nil { isBasicAuth = true } else if isBasicAuth { - err = fmt.Errorf("basic auth requested via query arg, but no value provided via %s header", authHeader) - return + return isBasicAuth, username, name, groups, emails, authLevel, fmt.Errorf("basic auth requested via query arg, but no value provided via %s header", authHeader) } if isBasicAuth { username, name, groups, emails, authLevel, err = verifyBasicAuth(ctx, authHeader, authValue) - return + + return isBasicAuth, username, name, groups, emails, authLevel, err } userSession := ctx.GetSession() - username, name, groups, emails, authLevel, err = verifySessionCookie(ctx, targetURL, &userSession, refreshProfile, refreshProfileInterval) + if username, name, groups, emails, authLevel, err = verifySessionCookie(ctx, targetURL, &userSession, refreshProfile, refreshProfileInterval); err != nil { + return isBasicAuth, username, name, groups, emails, authLevel, err + } sessionUsername := ctx.Request.Header.PeekBytes(headerSessionUsername) if sessionUsername != nil && !strings.EqualFold(string(sessionUsername), username) { ctx.Logger.Warnf("Possible cookie hijack or attempt to bypass security detected destroying the session and sending 401 response") - err = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx) - if err != nil { + if err = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx); err != nil { ctx.Logger.Errorf("Unable to destroy user session after handler could not match them to their %s header: %s", headerSessionUsername, err) } - err = fmt.Errorf("could not match user %s to their %s header with a value of %s when visiting %s", username, headerSessionUsername, sessionUsername, targetURL.String()) + return isBasicAuth, username, name, groups, emails, authLevel, fmt.Errorf("could not match user %s to their %s header with a value of %s when visiting %s", username, headerSessionUsername, sessionUsername, targetURL.String()) } - return + return isBasicAuth, username, name, groups, emails, authLevel, err } // VerifyGET returns the handler verifying if a request is allowed to go through. @@ -479,7 +462,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); err != nil { + if err = updateActivityTimestamp(ctx, isBasicAuth); err != nil { ctx.Error(fmt.Errorf("unable to update last activity: %s", err), messageOperationFailed) return } @@ -502,7 +485,7 @@ func VerifyGET(cfg schema.AuthenticationBackendConfiguration) middlewares.Reques setForwardedHeaders(&ctx.Response.Header, username, name, groups, emails) } - if err := updateActivityTimestamp(ctx, isBasicAuth); 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 e7f99a8e7..66eb23dda 100644 --- a/internal/handlers/handler_verify_test.go +++ b/internal/handlers/handler_verify_test.go @@ -1314,3 +1314,67 @@ func TestShouldNotRedirectRequestsForBypassACLWhenInactiveForTooLong(t *testing. assert.Equal(t, fasthttp.StatusUnauthorized, mock.Ctx.Response.StatusCode()) assert.Nil(t, mock.Ctx.Response.Header.Peek("Location")) } + +func TestIsSessionInactiveTooLong(t *testing.T) { + testCases := []struct { + name string + have *session.UserSession + now time.Time + inactivity time.Duration + expected bool + }{ + { + name: "ShouldNotBeInactiveTooLong", + have: &session.UserSession{Username: "john", LastActivity: 1656994960}, + now: time.Unix(1656994970, 0), + inactivity: time.Second * 90, + expected: false, + }, + { + name: "ShouldNotBeInactiveTooLongIfAnonymous", + have: &session.UserSession{Username: "", LastActivity: 1656994960}, + now: time.Unix(1656994990, 0), + inactivity: time.Second * 20, + expected: false, + }, + { + name: "ShouldNotBeInactiveTooLongIfRemembered", + have: &session.UserSession{Username: "john", LastActivity: 1656994960, KeepMeLoggedIn: true}, + now: time.Unix(1656994990, 0), + inactivity: time.Second * 20, + expected: false, + }, + { + name: "ShouldNotBeInactiveTooLongIfDisabled", + have: &session.UserSession{Username: "john", LastActivity: 1656994960}, + now: time.Unix(1656994990, 0), + inactivity: time.Second * 0, + expected: false, + }, + { + name: "ShouldBeInactiveTooLong", + have: &session.UserSession{Username: "john", LastActivity: 1656994960}, + now: time.Unix(4656994990, 0), + inactivity: time.Second * 1, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := mocks.NewMockAutheliaCtx(t) + + defer ctx.Close() + + ctx.Ctx.Configuration.Session.Inactivity = tc.inactivity + ctx.Ctx.Providers.SessionProvider = session.NewProvider(ctx.Ctx.Configuration.Session, nil) + + ctx.Clock.Set(tc.now) + ctx.Ctx.Clock = &ctx.Clock + + actual := isSessionInactiveTooLong(ctx.Ctx, tc.have, tc.have.Username == "") + + assert.Equal(t, tc.expected, actual) + }) + } +}