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 {