Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow session idle timeout to be configured on authentication. #10511

Merged
merged 3 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,21 @@ interface AuthConfiguration

IdentityService getIdentityService();

/**
* Should session ID be renewed on authentication.
* @return true if the session ID should be renewed on authentication
*/
boolean isSessionRenewedOnAuthentication();

/**
* Get the interval in seconds, which if non-zero, will be set
* with {@link javax.servlet.http.HttpSession#setMaxInactiveInterval(int)}
* when a session is newly authenticated
* @return An interval in seconds; or 0 to not set the interval
* on authentication; or a negative number to make the
* session never timeout after authentication.
*/
int getSessionMaxInactiveIntervalOnAuthentication();
gregw marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ public abstract class SecurityHandler extends HandlerWrapper implements Authenti
private final Map<String, String> _initParameters = new HashMap<>();
private LoginService _loginService;
private IdentityService _identityService;
private boolean _renewSession = true;
private boolean _renewSessionOnAuthentication = true;
private int _sessionMaxInactiveIntervalOnAuthentication = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a default of 0 is a good idea. If it is not set, then sessions that were previously configured - either programmatically or via web.xml - to timeout after x minutes are suddenly immortal after authentication.

Ooops, commented too soon. Now I've read the LoginAuthenticator code and it's the other way around: you can't cause sessions to become immortal after being authenticated. Immortality is indicated by a timeout <= 0, but you're only setting the maxInactiveInterval iff it's > 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated so that non-zero values are set on authentication, so we can now make a session immortal on authentication with a value of -1.

gregw marked this conversation as resolved.
Show resolved Hide resolved

static
{
Expand Down Expand Up @@ -433,7 +434,7 @@ protected boolean checkSecurity(Request request)
@Override
public boolean isSessionRenewedOnAuthentication()
{
return _renewSession;
return _renewSessionOnAuthentication;
}

/**
Expand All @@ -446,7 +447,26 @@ public boolean isSessionRenewedOnAuthentication()
*/
public void setSessionRenewedOnAuthentication(boolean renew)
{
_renewSession = renew;
_renewSessionOnAuthentication = renew;
}

@Override
public int getSessionMaxInactiveIntervalOnAuthentication()
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
return _sessionMaxInactiveIntervalOnAuthentication;
}

/**
* Set the interval in seconds, which if non-zero, will be set with
* {@link javax.servlet.http.HttpSession#setMaxInactiveInterval(int)}
* when a session is newly authenticated.
* @param seconds An interval in seconds; or 0 to not set the interval
* on authentication; or a negative number to make the
* session never timeout after authentication.
*/
public void setSessionMaxInactiveIntervalOnAuthentication(int seconds)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
_sessionMaxInactiveIntervalOnAuthentication = seconds;
}

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,10 @@ public boolean isSessionRenewedOnAuthentication()
{
return _configuration.isSessionRenewedOnAuthentication();
}

@Override
public int getSessionMaxInactiveIntervalOnAuthentication()
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
return _configuration.getSessionMaxInactiveIntervalOnAuthentication();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ public abstract class LoginAuthenticator implements Authenticator

protected LoginService _loginService;
protected IdentityService _identityService;
private boolean _renewSession;
private boolean _sessionRenewedOnAuthentication;
private int _sessionMaxInactiveIntervalOnAuthentication;

protected LoginAuthenticator()
{
Expand Down Expand Up @@ -88,7 +89,8 @@ public void setConfiguration(AuthConfiguration configuration)
_identityService = configuration.getIdentityService();
if (_identityService == null)
throw new IllegalStateException("No IdentityService for " + this + " in " + configuration);
_renewSession = configuration.isSessionRenewedOnAuthentication();
_sessionRenewedOnAuthentication = configuration.isSessionRenewedOnAuthentication();
_sessionMaxInactiveIntervalOnAuthentication = configuration.getSessionMaxInactiveIntervalOnAuthentication();
}

public LoginService getLoginService()
Expand All @@ -110,35 +112,41 @@ public LoginService getLoginService()
*/
protected HttpSession renewSession(HttpServletRequest request, HttpServletResponse response)
{
HttpSession httpSession = request.getSession(false);
HttpSession session = request.getSession(false);

if (_renewSession && httpSession != null)
if (session != null && (_sessionRenewedOnAuthentication || _sessionMaxInactiveIntervalOnAuthentication != 0))
{
synchronized (httpSession)
synchronized (session)
{
//if we should renew sessions, and there is an existing session that may have been seen by non-authenticated users
//(indicated by SESSION_SECURED not being set on the session) then we should change id
if (httpSession.getAttribute(Session.SESSION_CREATED_SECURE) != Boolean.TRUE)
if (_sessionMaxInactiveIntervalOnAuthentication != 0)
session.setMaxInactiveInterval(_sessionMaxInactiveIntervalOnAuthentication < 0 ? -1 : _sessionMaxInactiveIntervalOnAuthentication);
if (_sessionRenewedOnAuthentication)
{
if (httpSession instanceof Session)
//if we should renew sessions, and there is an existing session that may have been seen by non-authenticated users
//(indicated by SESSION_SECURED not being set on the session) then we should change id
if (session.getAttribute(Session.SESSION_CREATED_SECURE) != Boolean.TRUE)
{
Session s = (Session)httpSession;
String oldId = s.getId();
s.renewId(request);
s.setAttribute(Session.SESSION_CREATED_SECURE, Boolean.TRUE);
if (s.isIdChanged() && (response instanceof Response))
((Response)response).replaceCookie(s.getSessionHandler().getSessionCookie(s, request.getContextPath(), request.isSecure()));
if (LOG.isDebugEnabled())
LOG.debug("renew {}->{}", oldId, s.getId());
if (session instanceof Session)
{
Session s = (Session)session;
String oldId = s.getId();
s.renewId(request);
s.setAttribute(Session.SESSION_CREATED_SECURE, Boolean.TRUE);
if (s.isIdChanged() && (response instanceof Response))
((Response)response).replaceCookie(s.getSessionHandler().getSessionCookie(s, request.getContextPath(), request.isSecure()));
if (LOG.isDebugEnabled())
LOG.debug("renew {}->{}", oldId, s.getId());
}
else
{
LOG.warn("Unable to renew session {}", session);
}
return session;
}
else
{
LOG.warn("Unable to renew session {}", httpSession);
}
return httpSession;
}
}
}
return httpSession;

return session;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.eclipse.jetty.server.handler.AbstractHandler;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.server.handler.HandlerWrapper;
import org.eclipse.jetty.server.session.Session;
import org.eclipse.jetty.server.session.SessionHandler;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.TypeUtil;
Expand All @@ -80,6 +81,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class ConstraintTest
Expand All @@ -89,6 +91,8 @@ public class ConstraintTest
private LocalConnector _connector;
private ConstraintSecurityHandler _security;
private HttpConfiguration _config;
private ContextHandler _contextHandler;
private SessionHandler _sessionhandler;
private Constraint _forbidConstraint;
private Constraint _authAnyRoleConstraint;
private Constraint _authAdminConstraint;
Expand All @@ -106,8 +110,8 @@ public void setupServer()
_config = _connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration();
_server.setConnectors(new Connector[]{_connector});

ContextHandler contextHandler = new ContextHandler();
SessionHandler sessionHandler = new SessionHandler();
_contextHandler = new ContextHandler();
_sessionhandler = new SessionHandler();

TestLoginService loginService = new TestLoginService(TEST_REALM);

Expand All @@ -118,14 +122,14 @@ public void setupServer()
loginService.putUser("user3", new Password("password"), new String[]{"foo"});
loginService.putUser("user4", new Password("password"), new String[]{"A", "B", "C", "D"});

contextHandler.setContextPath("/ctx");
_server.setHandler(contextHandler);
contextHandler.setHandler(sessionHandler);
_contextHandler.setContextPath("/ctx");
_server.setHandler(_contextHandler);
_contextHandler.setHandler(_sessionhandler);

_server.addBean(loginService);

_security = new ConstraintSecurityHandler();
sessionHandler.setHandler(_security);
_sessionhandler.setHandler(_security);
RequestHandler requestHandler = new RequestHandler(new String[]{"user", "user4"}, new String[]{"user", "foo"});
_security.setHandler(requestHandler);

Expand Down Expand Up @@ -1189,6 +1193,93 @@ public void testFormRedirect() throws Exception
assertThat(response, not(containsString("JSESSIONID=" + session)));
}

public static Stream<Arguments> onAuthenticationTests()
{
return Stream.of(
Arguments.of(false, 0),
Arguments.of(false, -1),
Arguments.of(false, 2400),
Arguments.of(true, 0),
Arguments.of(true, -1),
Arguments.of(true, 2400)
);
}

@ParameterizedTest
@MethodSource("onAuthenticationTests")
public void testSessionOnAuthentication(boolean sessionRenewOnAuthentication, int sessionMaxInactiveIntervalOnAuthentication) throws Exception
{
final int UNAUTH_SECONDS = 1200;

// Use a FormAuthenticator as an example of session authentication
_security.setAuthenticator(new FormAuthenticator("/testLoginPage", "/testErrorPage", false));

_sessionhandler.setMaxInactiveInterval(UNAUTH_SECONDS);
_security.setSessionRenewedOnAuthentication(sessionRenewOnAuthentication);
_security.setSessionMaxInactiveIntervalOnAuthentication(sessionMaxInactiveIntervalOnAuthentication);
_server.start();

String response;

response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n\r\n");
assertThat(response, containsString(" 302 Found"));
assertThat(response, containsString("/ctx/testLoginPage"));
assertThat(response, containsString("JSESSIONID="));
String sessionId = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx"));

response = _connector.getResponse("GET /ctx/testLoginPage HTTP/1.0\r\n" +
"Cookie: JSESSIONID=" + sessionId + "\r\n" +
"\r\n");
assertThat(response, containsString(" 200 OK"));
assertThat(response, containsString("URI=/ctx/testLoginPage"));
assertThat(response, not(containsString("JSESSIONID=" + sessionId)));

response = _connector.getResponse("POST /ctx/j_security_check HTTP/1.0\r\n" +
"Cookie: JSESSIONID=" + sessionId + "\r\n" +
"Content-Type: application/x-www-form-urlencoded\r\n" +
"Content-Length: 35\r\n" +
"\r\n" +
"j_username=user&j_password=password");
assertThat(response, startsWith("HTTP/1.1 302 "));
assertThat(response, containsString("Location"));
assertThat(response, containsString("/ctx/auth/info"));

if (sessionRenewOnAuthentication)
{
// check session ID has changed.
assertNull(_sessionhandler.getSession(sessionId));
assertThat(response, containsString("Set-Cookie:"));
assertThat(response, containsString("JSESSIONID="));
assertThat(response, not(containsString("JSESSIONID=" + sessionId)));
sessionId = response.substring(response.indexOf("JSESSIONID=") + 11, response.indexOf("; Path=/ctx"));
}
else
{
// check session ID has not changed.
assertThat(response, not(containsString("Set-Cookie:")));
assertThat(response, not(containsString("JSESSIONID=")));
}

if (sessionMaxInactiveIntervalOnAuthentication == 0)
{
// check max interval has not been updated
Session session = _sessionhandler.getSession(_sessionhandler.getSessionIdManager().getId(sessionId));
assertThat(session.getMaxInactiveInterval(), is(UNAUTH_SECONDS));
}
else
{
// check max interval has not been updated
Session session = _sessionhandler.getSession(_sessionhandler.getSessionIdManager().getId(sessionId));
assertThat(session.getMaxInactiveInterval(), is(sessionMaxInactiveIntervalOnAuthentication));
}

// check session still there.
response = _connector.getResponse("GET /ctx/auth/info HTTP/1.0\r\n" +
"Cookie: JSESSIONID=" + sessionId + "\r\n" +
"\r\n");
assertThat(response, startsWith("HTTP/1.1 200 OK"));
}
gregw marked this conversation as resolved.
Show resolved Hide resolved

@Test
public void testFormPostRedirect() throws Exception
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,7 @@ public void setSessionIdManager(SessionIdManager metaManager)
/**
* Sets the max period of inactivity, after which the session is invalidated, in seconds.
*
* @param seconds the max inactivity period, in seconds.
* @param seconds the max inactivity period, in seconds. If less than or equal to zero, then the session is immortal
* @see #getMaxInactiveInterval()
*/
public void setMaxInactiveInterval(int seconds)
Expand Down