Hold UpdateFilter authentication state per session (#6329)

UpdateFilter.authenticatedSuccessfully was an instance field on the
singleton filter, so once any super user authenticated at the first step
of the database-update wizard the flag stayed true for everyone. A
concurrent unauthenticated request could then POST straight to the
review-changes step and run the pending database updates on the strength
of another user's login.

Move the authenticated flag into the HttpSession so each session must
authenticate for itself. The shared update-in-progress state is left
global on purpose so multiple super users can still watch one running
update. Updates the E2E helpers to session scope, makes resetRequest carry
the session forward (a follow-up request is the same client), and adds a
regression test that a second session cannot reuse another session's
authentication.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: dkayiwa

web/src/main/java/org/openmrs/web/filter/update/UpdateFilter.java

@@ -80,6 +80,13 @@ public class UpdateFilter extends StartupFilter {
 
 	private static final String PROGRESS_VM_AJAXREQUEST = "updateProgress.vm.ajaxRequest";
 
+	/**
+	 * Session attribute set once a super user authenticates at the first step and checked on every
+	 * later step. Holding it on the session, rather than on the shared filter instance, stops one
+	 * user's successful authentication from being reused by another user's unauthenticated request.
+	 */
+	private static final String AUTHENTICATED_SUCCESSFULLY = "updateFilter.authenticatedSuccessfully";
+
 	/**
 	 * The model object behind this set of screens
 	 */
@@ -91,12 +98,6 @@ public class UpdateFilter extends StartupFilter {
 	 */
 	private static boolean updatesRequired = true;
 
-	/**
-	 * Used on all pages after the first to make sure the user isn't trying to cheat and do some url
-	 * magic to hack in.
-	 */
-	private volatile boolean authenticatedSuccessfully = false;
-
 	private UpdateFilterCompletion updateJob;
 
 	/**
@@ -157,8 +158,8 @@ protected synchronized void doPost(HttpServletRequest httpRequest, HttpServletRe
 			log.debug("Attempting to authenticate user: " + username);
 			if (authenticateAsSuperUser(username, password)) {
 				log.debug("Authentication successful.  Redirecting to 'reviewupdates' page.");
-				// set a variable so we know that the user started here
-				authenticatedSuccessfully = true;
+				// mark this session as authenticated so later steps can verify it
+				httpRequest.getSession().setAttribute(AUTHENTICATED_SUCCESSFULLY, Boolean.TRUE);
 
 				//Set variable to tell us whether updates are already in progress
 				referenceMap.put("isDatabaseUpdateInProgress", isDatabaseUpdateInProgress);
@@ -211,7 +212,7 @@ protected synchronized void doPost(HttpServletRequest httpRequest, HttpServletRe
 		// step two of wizard in case if there were some warnings
 		else if (REVIEW_CHANGES.equals(page)) {
 
-			if (!authenticatedSuccessfully) {
+			if (!Boolean.TRUE.equals(httpRequest.getSession().getAttribute(AUTHENTICATED_SUCCESSFULLY))) {
 				// throw the user back to the main page because they are cheating
 				renderTemplate(DEFAULT_PAGE, referenceMap, httpResponse);
 				return;

web/src/test/java/org/openmrs/web/filter/update/UpdateFilterE2ETest.java

@@ -40,6 +40,9 @@ class UpdateFilterE2ETest {
 
 	private MockHttpServletResponse response;
 
+	/** Must match {@code UpdateFilter.AUTHENTICATED_SUCCESSFULLY}. */
+	private static final String AUTHENTICATED_SUCCESSFULLY_ATTR = "updateFilter.authenticatedSuccessfully";
+
 	/**
 	 * Testable subclass that overrides renderTemplate and authenticateAsSuperUser to avoid Velocity
 	 * rendering and database access.
@@ -77,7 +80,6 @@ void setup() throws Exception {
 				this.changes = null;
 			}
 		});
-		setAuthenticatedSuccessfully(false);
 		setDatabaseUpdateInProgress(false);
 		UpdateFilter.setLockReleased(false);
 
@@ -88,7 +90,6 @@ void setup() throws Exception {
 	@AfterEach
 	void cleanup() throws Exception {
 		setDatabaseUpdateInProgress(false);
-		setAuthenticatedSuccessfully(false);
 		UpdateFilter.setUpdatesRequired(true);
 		UpdateFilter.setLockReleased(false);
 	}
@@ -184,6 +185,23 @@ void reviewChangesPage_shouldRedirectToMaintenanceIfNotAuthenticated() throws Ex
 		assertEquals("maintenance.vm", filter.lastRenderedTemplate);
 	}
 
+	@Test
+	void reviewChangesPage_shouldNotReuseAnotherSessionsAuthentication() throws Exception {
+		// one super user authenticates in their own session
+		filter.authResult = true;
+		MockHttpServletRequest authRequest = new MockHttpServletRequest();
+		authRequest.setParameter("page", "maintenance.vm");
+		authRequest.setParameter("username", "admin");
+		authRequest.setParameter("password", "Admin123");
+		filter.doPost(authRequest, new MockHttpServletResponse());
+
+		// a different, unauthenticated session must not inherit that authentication
+		request.setParameter("page", "reviewchanges.vm");
+		filter.doPost(request, response);
+
+		assertEquals("maintenance.vm", filter.lastRenderedTemplate);
+	}
+
 	@Test
 	void reviewChangesPage_shouldRenderReviewChangesWhenAuthenticated() throws Exception {
 		setAuthenticatedSuccessfully(true);
@@ -431,20 +449,23 @@ private void clearErrors() throws Exception {
 	}
 
 	private void resetRequest() {
+		// carry the session forward so a follow-up request represents the same client (same session)
+		MockHttpServletRequest previous = request;
 		request = new MockHttpServletRequest();
+		request.setSession(previous.getSession());
 		response = new MockHttpServletResponse();
 	}
 
-	private void setAuthenticatedSuccessfully(boolean value) throws Exception {
-		Field field = UpdateFilter.class.getDeclaredField("authenticatedSuccessfully");
-		field.setAccessible(true);
-		field.setBoolean(filter, value);
+	private void setAuthenticatedSuccessfully(boolean value) {
+		if (value) {
+			request.getSession().setAttribute(AUTHENTICATED_SUCCESSFULLY_ATTR, Boolean.TRUE);
+		} else {
+			request.getSession().removeAttribute(AUTHENTICATED_SUCCESSFULLY_ATTR);
+		}
 	}
 
-	private boolean getAuthenticatedSuccessfully() throws Exception {
-		Field field = UpdateFilter.class.getDeclaredField("authenticatedSuccessfully");
-		field.setAccessible(true);
-		return field.getBoolean(filter);
+	private boolean getAuthenticatedSuccessfully() {
+		return Boolean.TRUE.equals(request.getSession().getAttribute(AUTHENTICATED_SUCCESSFULLY_ATTR));
 	}
 
 	private void setDatabaseUpdateInProgress(boolean value) throws Exception {