This is an automated email from the ASF dual-hosted git repository. dashorst pushed a commit to branch wicket-10.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit d6587d297175f8501f7873c370e5b33f45ee6750 Author: Emond Papegaaij <[email protected]> AuthorDate: Thu Sep 24 20:14:36 2026 +0200 Renew the session id on every sign in AuthenticatedWebSession#signIn(String, String) renewed the container session id inside signedIn.compareAndSet(false, true), the guard that keeps concurrent sign ins from binding the session more than once. The renewal therefore ran only on the false to true transition of the flag: a session that was already signed in and authenticated again kept the id it had, and signIn returned true either way, so neither the caller nor the application could tell that the renewal had been skipped. Renewing the id on login is what Session#changeSessionId() documents itself as being for, and every authentication is a login: a step up prompt before a sensitive action, an account switch, or a sign in form that stays reachable once the user is signed in all reach signIn on a session whose flag is already set. changeSessionId() now sits outside the compare and set and runs on every successful authentication. bind() stays inside it, so the session is still bound exactly once, and the two keep their order: on a session that has no container session yet the renewal is a no-op and bind() has one created. An application that calls signIn on every request now renews the id on every request. Nothing in Wicket does that - SignInPanel#onConfigure signs in only when the session is not signed in, and UsernamePasswordPanel only on form submit. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../authentication/AuthenticatedWebSession.java | 12 +- .../authentication/SignInSessionIdTest.java | 182 +++++++++++++++++++++ 2 files changed, 192 insertions(+), 2 deletions(-) diff --git a/wicket-auth-roles/src/main/java/org/apache/wicket/authroles/authentication/AuthenticatedWebSession.java b/wicket-auth-roles/src/main/java/org/apache/wicket/authroles/authentication/AuthenticatedWebSession.java index 5438cfba95..8952b8c571 100644 --- a/wicket-auth-roles/src/main/java/org/apache/wicket/authroles/authentication/AuthenticatedWebSession.java +++ b/wicket-auth-roles/src/main/java/org/apache/wicket/authroles/authentication/AuthenticatedWebSession.java @@ -56,6 +56,10 @@ public abstract class AuthenticatedWebSession extends AbstractAuthenticatedWebSe /** * Try to sign in the user. It'll call {@link #authenticate(String, String)} to do the real work * and that is what you need to subclass to provide your own authentication mechanism. + * <p> + * Every successful authentication renews the session id through + * {@link Session#changeSessionId()}, not only the first one, so that an id that was known + * before the authentication is never carried into the authenticated session. * * @param username * @param password @@ -69,10 +73,14 @@ public abstract class AuthenticatedWebSession extends AbstractAuthenticatedWebSe { signOut(); } - else if (authenticated && signedIn.compareAndSet(false, true)) + else if (authenticated) { changeSessionId(); - bind(); + + if (signedIn.compareAndSet(false, true)) + { + bind(); + } } return authenticated; } diff --git a/wicket-auth-roles/src/test/java/org/apache/wicket/authroles/authentication/SignInSessionIdTest.java b/wicket-auth-roles/src/test/java/org/apache/wicket/authroles/authentication/SignInSessionIdTest.java new file mode 100644 index 0000000000..f8d6e57da2 --- /dev/null +++ b/wicket-auth-roles/src/test/java/org/apache/wicket/authroles/authentication/SignInSessionIdTest.java @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.authroles.authentication; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.apache.wicket.Page; +import org.apache.wicket.authroles.authentication.pages.UsernamePasswordPage; +import org.apache.wicket.authroles.authorization.strategies.role.Roles; +import org.apache.wicket.markup.html.WebPage; +import org.apache.wicket.protocol.http.mock.MockHttpSession; +import org.apache.wicket.request.Request; +import org.apache.wicket.util.tester.DummyHomePage; +import org.apache.wicket.util.tester.WicketTester; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Tests that {@link AuthenticatedWebSession#signIn(String, String)} renews the session id, so that + * an id that was known before the authentication cannot be used afterwards. + */ +class SignInSessionIdTest +{ + private static final String PASSWORD = "secret"; + + private WicketTester tester; + + private TestSession session; + + /** + * Gives the session a container session of its own, so that it is not temporary and + * {@link org.apache.wicket.Session#changeSessionId()} is not a no-op. + */ + @BeforeEach + void bindTheSession() + { + tester = new WicketTester(new TestApplication()); + + MockHttpSession httpSession = (MockHttpSession)tester.getRequest().getSession(); + httpSession.setTemporary(false); + + session = (TestSession)tester.getSession(); + session.bind(); + } + + @AfterEach + void destroyTester() + { + tester.destroy(); + } + + /** + * The first authentication of a session renews its id. + */ + @Test + void firstSignInRenewsTheSessionId() + { + String anonymousId = session.getId(); + assertNotNull(anonymousId, "the session has to be bound for its id to mean anything"); + + assertTrue(session.signIn("user", PASSWORD)); + + assertEquals("user", session.getUser()); + assertNotEquals(anonymousId, session.getId(), + "the first authentication has to renew the session id"); + } + + /** + * So does every authentication after it. A session that is already signed in keeps its id + * otherwise, and whoever knew that id keeps a usable one for the session the second + * authentication signed in. + */ + @Test + void everyFurtherSignInRenewsTheSessionIdAgain() + { + assertTrue(session.signIn("first", PASSWORD)); + String firstId = session.getId(); + + assertTrue(session.signIn("second", PASSWORD)); + + assertEquals("second", session.getUser()); + assertTrue(session.isSignedIn()); + assertNotEquals(firstId, session.getId(), + "authenticating again has to renew the session id as well"); + } + + /** + * An authentication that fails still signs the session out, whatever it was before. + */ + @Test + void failedSignInSignsTheSessionOut() + { + assertTrue(session.signIn("user", PASSWORD)); + + assertFalse(session.signIn("user", "wrong")); + + assertFalse(session.isSignedIn()); + } + + /** + * A session that remembers who it authenticated last. + */ + public static class TestSession extends AuthenticatedWebSession + { + private static final long serialVersionUID = 1L; + + private String user; + + /** + * Construct. + * + * @param request + * the current web request + */ + public TestSession(Request request) + { + super(request); + } + + @Override + protected boolean authenticate(String username, String password) + { + if (PASSWORD.equals(password)) + { + user = username; + return true; + } + return false; + } + + @Override + public Roles getRoles() + { + return isSignedIn() ? new Roles("USER") : new Roles(); + } + + String getUser() + { + return user; + } + } + + private static class TestApplication extends AuthenticatedWebApplication + { + @Override + protected Class<? extends AbstractAuthenticatedWebSession> getWebSessionClass() + { + return TestSession.class; + } + + @Override + protected Class<? extends WebPage> getSignInPageClass() + { + return UsernamePasswordPage.class; + } + + @Override + public Class<? extends Page> getHomePage() + { + return DummyHomePage.class; + } + } +}
