Hold UpdateFilter authentication state per session

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 {