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

@@ -128,24 +128,16 @@ func setForwardedHeaders(headers *fasthttp.ResponseHeader, username, name string
 	}
 }
 
-// hasUserBeenInactiveTooLong checks whether the user has been inactive for too long.
+func isSessionInactiveTooLong(ctx *middlewares.AutheliaCtx, userSession *session.UserSession, isUserAnonymous bool) (isInactiveTooLong bool) {
-func hasUserBeenInactiveTooLong(ctx *middlewares.AutheliaCtx) (bool, error) { //nolint:unparam
+	if userSession.KeepMeLoggedIn || isUserAnonymous || int64(ctx.Providers.SessionProvider.Inactivity.Seconds()) == 0 {
-	maxInactivityPeriod := int64(ctx.Providers.SessionProvider.Inactivity.Seconds())
+		return false
-	if maxInactivityPeriod == 0 {
-		return false, nil
 	}
 
-	lastActivity := ctx.GetSession().LastActivity
+	isInactiveTooLong = time.Unix(userSession.LastActivity, 0).Add(ctx.Providers.SessionProvider.Inactivity).Before(ctx.Clock.Now())
-	inactivityPeriod := ctx.Clock.Now().Unix() - lastActivity
 
-	ctx.Logger.Tracef("Inactivity report: Inactivity=%d, MaxInactivity=%d",
+	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()))
-		inactivityPeriod, maxInactivityPeriod)
 
-	if inactivityPeriod > maxInactivityPeriod {
+	return isInactiveTooLong
-		return true, nil
-	}
-
-	return false, nil
 }
 
 // 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 {
+	if isSessionInactiveTooLong(ctx, userSession, 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 inactiveLongEnough {
 		// Destroy the session a new one will be regenerated on next request.
-			err := ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx)
+		if err = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx); err != nil {
-			if 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)
-				return "", "", nil, nil, authentication.NotAuthenticated, fmt.Errorf("unable to destroy user session after long inactivity: %s", err)
 		}
 
-			ctx.Logger.Warnf("User %s has been inactive for too long", userSession.Username)
+		ctx.Logger.Warnf("Session destroyed for user '%s' after exceeding configured session inactivity and not being marked as remembered", userSession.Username)
 
 		return "", "", nil, nil, authentication.NotAuthenticated, nil
 	}
-	}
 
-	err = verifySessionHasUpToDateProfile(ctx, targetURL, userSession, refreshProfile, refreshProfileInterval)
+	if err = verifySessionHasUpToDateProfile(ctx, targetURL, userSession, refreshProfile, refreshProfileInterval); err != nil {
-	if err != nil {
 		if err == authentication.ErrUserNotFound {
-			err = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx)
+			if err = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx); err != nil {
-			if err != nil {
+				ctx.Logger.Errorf("Unable to destroy user session after provider refresh didn't find the user: %v", err)
-				ctx.Logger.Errorf("Unable to destroy user session after provider refresh didn't find the user: %s", 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 isBasicAuth, username, name, groups, emails, authLevel, fmt.Errorf("basic auth requested via query arg, but no value provided via %s header", authHeader)
-		return
 	}
 
 	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 = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx); err != nil {
-		if 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)
 		}
 	}

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)
+		})
+	}
+}