Skip to content

RCM-124 - Cohort Builder pages fails with DWR javascript errors #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mseaton merged 3 commits into from
Jul 21, 2026
Merged

RCM-124 - Cohort Builder pages fails with DWR javascript errors #54

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe7ee49
RCM-124 - Cohort Builder pages fails with DWR javascript errors
mseaton Jul 20, 2026
00fdc0c
RCM-124 - Add config.xml/annotation completeness test, bump min Java …
mseaton Jul 20, 2026
94d27ad
RCM-124 - Update legacyui dependencies
mseaton Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion omod/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
<dependency>
<groupId>org.openmrs.module</groupId>
<artifactId>legacyui-omod</artifactId>
<version>1.0</version>
<version>2.1.0</version>

@dkayiwa dkayiwa Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiling against 2.1.0 is right (it's the first release with RequirePrivilege), but the runtime half of this fix is openmrs/openmrs-module-legacyui#268, which isn't in any legacyui release yet. The 2.1.0 DwrAuthorizationFilter builds its allowlist from legacyui's own config.xml only. I initialized the filter straight from the published 2.1.0 jar to check: 100 entries, all legacyui's own scripts, nothing from this module. So under legacyui 2.1.0 every DWRCohortBuilderService.* call keeps returning the RCM-124 403 even with this merged, and the annotations added here are never read. Nothing to change in the code. I'd just record the minimum legacyui version in the ticket and in the release notes of whatever module release carries this, and hold that release until the legacyui side ships, so nobody upgrades against legacyui 2.1.0 expecting the cohort builder back.

@mseaton mseaton Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually bumping the required version up to 2.2.0-SNAPSHOT @dkayiwa , just to ensure the fix is in and these endpoints work

@dkayiwa dkayiwa Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good @mseaton. 2.2.0-SNAPSHOT does include the LUI-212 fix (openmrs/openmrs-module-legacyui#268 merged earlier today), so compiling against it guarantees the annotations and the filter line up. Two things worth keeping on the radar for release time:

  1. The maven-release-plugin will refuse to release this module while legacyuiVersion points at a SNAPSHOT, so a legacyui 2.2.0 release needs to be cut first and the property bumped to it. Conveniently, that also enforces the "hold this release until the legacyui side ships" part automatically.

  2. The pom bump only covers compile time. In config.xml, legacyui is still just an <aware_of_module> with no version constraint, so nothing at runtime stops the module from starting against legacyui 2.1.0, where the DWR endpoints keep returning the 403. Since legacyui is deliberately optional here, I'd still record legacyui >= 2.2.0 in the RCM-124 ticket and the release notes.

<scope>provided</scope>
</dependency>
<dependency>
Expand Down
29 changes: 27 additions & 2 deletions omod/src/main/java/org/openmrs/web/dwr/DWRCohortBuilderService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,19 @@
import org.openmrs.reporting.ReportObject;
import org.openmrs.reporting.ReportObjectService;
import org.openmrs.util.OpenmrsUtil;
import org.openmrs.util.PrivilegeConstants;
import org.openmrs.util.ReportingcompatibilityUtil;
import org.openmrs.web.controller.analysis.CohortBuilderController;
import org.openmrs.web.security.RequirePrivilege;

public class DWRCohortBuilderService {


// matches the privilege cohortBuilder.jsp itself requires via <openmrs:require>
private static final String VIEW_PATIENT_COHORTS = "View Patient Cohorts";

protected final Log log = LogFactory.getLog(getClass());


@RequirePrivilege(VIEW_PATIENT_COHORTS)
public Integer getResultCountForFilterId(Integer filterId) {
ReportObjectService rs = (ReportObjectService) Context.getService(ReportObjectService.class);
PatientFilter pf = rs.getPatientFilterById(filterId);
Expand All @@ -64,30 +70,35 @@ private CohortSearchHistory getMySearchHistory() {
* @param index
* @return the number of patients in the resulting PatientSet
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public Integer getResultCountForSearch(int index) {
CohortSearchHistory history = getMySearchHistory();
Cohort ps = history.getPatientSet(index, null);
return ps.size();
}

@RequirePrivilege(VIEW_PATIENT_COHORTS)
public Cohort getResultForSearch(int index) {
CohortSearchHistory history = getMySearchHistory();
Cohort ps = history.getPatientSet(index, null);
return ps;
}

@RequirePrivilege(VIEW_PATIENT_COHORTS)
public Cohort getResultCombineWithAnd() {
CohortSearchHistory history = getMySearchHistory();
Cohort ps = history.getPatientSetCombineWithAnd(new EvaluationContext());
return ps;
}

@RequirePrivilege(VIEW_PATIENT_COHORTS)
public Cohort getResultCombineWithOr() {
CohortSearchHistory history = getMySearchHistory();
Cohort ps = history.getPatientSetCombineWithOr(new EvaluationContext());
return ps;
}

@RequirePrivilege(VIEW_PATIENT_COHORTS)
public Cohort getLastResult() {
CohortSearchHistory history = getMySearchHistory();
if (history != null) {
Expand All @@ -98,6 +109,7 @@ public Cohort getLastResult() {
new Cohort();
}

@RequirePrivilege(VIEW_PATIENT_COHORTS)
public List<ListItem> getSavedSearches(boolean includeParameterized) {
List<ListItem> ret = new ArrayList<ListItem>();
ReportObjectService rs = (ReportObjectService) Context.getService(ReportObjectService.class);
Expand All @@ -115,6 +127,7 @@ public List<ListItem> getSavedSearches(boolean includeParameterized) {
return ret;
}

@RequirePrivilege(VIEW_PATIENT_COHORTS)
public List<ListItem> getSavedFilters() {
List<ListItem> ret = new ArrayList<ListItem>();
ReportObjectService rs = (ReportObjectService) Context.getService(ReportObjectService.class);
Expand All @@ -134,6 +147,7 @@ public List<ListItem> getSavedFilters() {
*
* @return
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public List<ListItem> getSavedCohorts() {
List<ListItem> ret = new ArrayList<ListItem>();
List<org.openmrs.Cohort> cohorts = Context.getCohortService().getAllCohorts();
Expand All @@ -154,6 +168,7 @@ public List<ListItem> getSavedCohorts() {
* @param filterId
* @return
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public String getFilterResultAsCommaSeparatedIds(Integer filterId) {
ReportObjectService rs = (ReportObjectService) Context.getService(ReportObjectService.class);
PatientFilter pf = rs.getPatientFilterById(filterId);
Expand All @@ -169,6 +184,7 @@ public String getFilterResultAsCommaSeparatedIds(Integer filterId) {
* @param cohortId
* @return
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public String getCohortAsCommaSeparatedIds(Integer cohortId) {
Cohort c = ReportingcompatibilityUtil.convert(Context.getCohortService().getCohort(cohortId));
if (c == null)
Expand All @@ -182,6 +198,7 @@ public String getCohortAsCommaSeparatedIds(Integer cohortId) {
*
* @return
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public List<ListItem> getSearchHistories() {
List<ListItem> ret = new ArrayList<ListItem>();
ReportObjectService rs = (ReportObjectService) Context.getService(ReportObjectService.class);
Expand All @@ -202,6 +219,7 @@ public List<ListItem> getSearchHistories() {
* @param name
* @param description
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public void saveSearchHistory(String name, String description) {
CohortSearchHistory history = getMySearchHistory();
if (history.getReportObjectId() != null)
Expand All @@ -217,6 +235,7 @@ public void saveSearchHistory(String name, String description) {
*
* @param id
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public void loadSearchHistory(Integer id) {
ReportObjectService rs = (ReportObjectService) Context.getService(ReportObjectService.class);
CohortBuilderController.setVolatileUserData("CohortBuilderSearchHistory", rs.getSearchHistory(id));
Expand All @@ -229,6 +248,7 @@ public void loadSearchHistory(Integer id) {
* @param description The description to give the saved filter
* @param indexInHistory The index into the authenticated user's search history
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public Boolean saveHistoryElement(String name, String description, Integer indexInHistory) {
CohortSearchHistory history = getMySearchHistory();
try {
Expand Down Expand Up @@ -259,6 +279,8 @@ public Boolean saveHistoryElement(String name, String description, Integer index
* @param description
* @param commaSeparatedIds
*/
// matches CohortService#saveCohort's own @Authorized requirement
@RequirePrivilege({ PrivilegeConstants.ADD_COHORTS, PrivilegeConstants.EDIT_COHORTS })
public void saveCohort(String name, String description, String commaSeparatedIds) {
Set<Integer> ids = new HashSet<Integer>(OpenmrsUtil.delimitedStringToIntegerList(commaSeparatedIds, ","));
org.openmrs.Cohort cohort = new org.openmrs.Cohort();
Expand All @@ -272,6 +294,7 @@ public void saveCohort(String name, String description, String commaSeparatedIds
* This isn't really useful because most of the properties don't have DWR converters. I'm
* leaving it here in case I get to work on it later.
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public CohortSearchHistory getUserSearchHistory() {
return getMySearchHistory();
}
Expand All @@ -289,6 +312,7 @@ public CohortSearchHistory getUserSearchHistory() {
* @return Vector<Parameter> containing all Parameters that need to be provided to evaluate the
* input cohortSpecification
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public Vector<Parameter> getMissingParameters(String cohortSpecification) {
Vector<Parameter> ret = new Vector<Parameter>();
CohortDefinition def = CohortUtil.parse(cohortSpecification);
Expand All @@ -306,6 +330,7 @@ public Vector<Parameter> getMissingParameters(String cohortSpecification) {
* @param cohortSpecification - This input String represents the Cohort Definition to evaluate
* @return Cohort - The Cohort of patients that are returned
*/
@RequirePrivilege(VIEW_PATIENT_COHORTS)
public Cohort evaluateCohortDefinition(String cohortSpecification, Map<Parameter, Object> parameterValues) {
CohortDefinition def = CohortUtil.parse(cohortSpecification);
EvaluationContext evalContext = new EvaluationContext();
Expand Down
8 changes: 6 additions & 2 deletions omod/src/main/java/org/openmrs/web/dwr/DWRPatientSetService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,15 @@
import org.openmrs.Patient;
import org.openmrs.api.PatientService;
import org.openmrs.api.context.Context;
import org.openmrs.util.PrivilegeConstants;
import org.openmrs.web.security.RequirePrivilege;

public class DWRPatientSetService {

protected final Log log = LogFactory.getLog(getClass());


// matches PatientService#getPatient's own @Authorized requirement
@RequirePrivilege(PrivilegeConstants.GET_PATIENTS)
public Vector<PatientListItem> getPatients(String patientIds) {
Vector<PatientListItem> ret = new Vector<PatientListItem>();
List<Integer> ptIds = new ArrayList<Integer>();
Expand Down
37 changes: 15 additions & 22 deletions omod/src/main/resources/config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,31 +33,12 @@
<allow>
<create creator="new" javascript="DWRPatientSetService">
<param name="class" value="org.openmrs.web.dwr.DWRPatientSetService"/>
</create>
<create creator="new" javascript="DWRCohortBuilderService">
<param name="class" value="org.openmrs.web.dwr.DWRCohortBuilderService"/>
<include method="getPatients"/>

@dkayiwa dkayiwa Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the LUI-212 filter is in play it fails open, so a method added to this list later without @RequirePrivilege ships silently unenforced at the request boundary (nothing throws, the only trace is a startup WARN in legacyui's log). The enforcement lives in legacyui, but the parity is testable in this repo: a small unit test could parse config.xml and assert that every <include>d method resolves to an annotated method on the declared class, and that the file keeps exactly one <dwr> block (WebModuleUtil merges only the first one into dwr-modules.xml, which is how the old second block here got silently dropped for years). Optional, but it would keep the next addition honest.

@mseaton mseaton Jul 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added DwrConfigXmlTest (00fdc0c): asserts config.xml has exactly one block, and that every d method resolves to a @RequirePrivilege-annotated method, mirroring DwrAuthorizationFilter's own scanDwrCreates/findMethodAnnotation logic. Verified it fails when either bug is reintroduced (duplicate block, or an unannotated method). Needed Java 8 (diamond operator, try-with-resources), so bumped the module's min compiler source/target from 1.5 to 1.8 in the same commit.

</create>
<convert match="org.openmrs.cohort.Cohort" converter="bean"/>
</allow>
</dwr>

<globalProperty>
<property>reportingcompatibility.patientLinkUrl</property>
<defaultValue>patientDashboard.form</defaultValue>
<description>The link url for a particular patient to view from the cohort builder</description>
</globalProperty>

<globalProperty>
<property>reportingcompatibility.data_export_batch_size</property>
<defaultValue>7500</defaultValue>
<description>The number of patients to export at a time in a data export. The larger this number the faster and more memory that is used. The smaller this number the slower and less memory is used.</description>
</globalProperty>

<dwr>
<allow>
<convert converter="bean" match="org.openmrs.report.Parameter"/>
<convert converter="bean" match="org.openmrs.cohort.CohortSearchHistory"/>

<create creator="new" javascript="DWRCohortBuilderService">
<param name="class" value="org.openmrs.web.dwr.DWRCohortBuilderService" />
<include method="getMissingParameters"/>
Expand All @@ -82,7 +63,19 @@
</create>
</allow>
</dwr>


<globalProperty>
<property>reportingcompatibility.patientLinkUrl</property>
<defaultValue>patientDashboard.form</defaultValue>
<description>The link url for a particular patient to view from the cohort builder</description>
</globalProperty>

<globalProperty>
<property>reportingcompatibility.data_export_batch_size</property>
<defaultValue>7500</defaultValue>
<description>The number of patients to export at a time in a data export. The larger this number the faster and more memory that is used. The smaller this number the slower and less memory is used.</description>
</globalProperty>

<servlet>
<servlet-name>dataExportServlet</servlet-name>
<servlet-class>org.openmrs.web.servlet.DataExportServlet</servlet-class>
Expand Down