From f0027d67ea4132dbe6083d704435814cdf4175d9 Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 15 Jul 2026 18:25:19 +0300 Subject: [PATCH] Hold UpdateFilter authentication state per session 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 --- .../web/filter/update/UpdateFilter.java | 19 +++++---- .../filter/update/UpdateFilterE2ETest.java | 41 ++++++++++++++----- 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/web/src/main/java/org/openmrs/web/filter/update/UpdateFilter.java b/web/src/main/java/org/openmrs/web/filter/update/UpdateFilter.java index 669c0ea9e800..d7dd8cafe53f 100644 --- a/web/src/main/java/org/openmrs/web/filter/update/UpdateFilter.java +++ b/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; diff --git a/web/src/test/java/org/openmrs/web/filter/update/UpdateFilterE2ETest.java b/web/src/test/java/org/openmrs/web/filter/update/UpdateFilterE2ETest.java index 447d05abb1ba..d113d9592a35 100644 --- a/web/src/test/java/org/openmrs/web/filter/update/UpdateFilterE2ETest.java +++ b/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 {