LUI-212 - DWR requests in reportingcompatibility fail with a Forbidde…

[mseaton]

…n exception

[dkayiwa]

These messages still describe the old fail-closed behavior. With this PR, unannotated methods are passed through, not rejected with 403 (the class javadoc itself now says the warning "is not enforcement"). If this merges as-is, the first thing an operator sees after upgrading to fix LUI-212 is a WARN listing every reportingcompatibility DWR method as "will be rejected with 403", claiming the very breakage this PR removes is still in place, and anyone auditing DWR exposure from the logs will conclude those endpoints are blocked when they are in fact open. Two nearby spots have the same staleness: the fallback message in loadFallbackOnce says "will allow no DWR calls" when an empty map now means the filter gates nothing (everything passes through), and the field javadoc on privilegesByScriptMethod still says unannotated lookups "fall back to authentication-only", which is also not what happens (they pass through with no auth at all). Worth fixing all three before merge:

Suggested change

	log.info("DwrAuthorizationFilter (re)loaded; annotated DWR methods: " + annotated.size()
	+ ", unannotated (will be rejected with 403): " + unannotated.size());
	if (!unannotated.isEmpty()) {
	log.warn("The following DWR methods are exposed without @RequirePrivilege and will be "
	+ "rejected with 403: " + unannotated.keySet());
	log.info("DwrAuthorizationFilter (re)loaded; annotated DWR methods: " + annotated.size()
	+ ", unannotated (not gated by this filter): " + unannotated.size());
	if (!unannotated.isEmpty()) {
	log.warn("The following DWR methods are exposed without @RequirePrivilege and are not gated "
	+ "by this filter; only their own service-layer checks apply: " + unannotated.keySet());

[mseaton]

Fixed in 4760e31 (all three spots): the log.info/log.warn in applyLoadedAnnotations, the loadFallbackOnce warning (now "will gate no DWR calls"), and the privilegesByScriptMethod field javadoc (now says unannotated lookups pass straight through with no auth check at all, not "fall back to authentication-only").

[dkayiwa]

Everything in DwrAuthorizationFilterTest still calls init(null), so only the classpath fallback ever runs under test. The path production actually takes (resolving dwr-modules.xml through the ServletContext, scanning it with the OpenmrsClassLoader, and this reload-on-mtime-change logic) has no coverage, and it's the part of this PR with real moving pieces. Since 2.1.0 already shipped a version of this filter whose production behavior diverged from what the tests exercised, I'd add a test for the primary path too. Not blocking on it, and it needs nothing new on the test classpath. I ran this shape against this branch and both tests pass:

public class DwrAuthorizationFilterPrimaryPathTest extends BaseModuleWebContextSensitiveTest {
	
	private File webappRoot;
	
	private File dwrXml;
	
	private DwrAuthorizationFilter filter;
	
	private RecordingFilterChain chain;
	
	@BeforeEach
	public void setUpFilter() throws Exception {
		webappRoot = Files.createTempDirectory("dwr-webapp").toFile();
		File webInf = new File(webappRoot, "WEB-INF");
		assertTrue(webInf.mkdirs() || webInf.isDirectory());
		dwrXml = new File(webInf, "dwr-modules.xml");
		writeDwrXml(false);
		// a "file:" resource base makes MockServletContext.getRealPath resolve into the temp dir
		MockServletContext servletContext = new MockServletContext("file:" + webappRoot.getAbsolutePath());
		filter = new DwrAuthorizationFilter();
		filter.init(new MockFilterConfig(servletContext, "dwrAuthorization"));
		chain = new RecordingFilterChain();
	}
	
	private void writeDwrXml(boolean includeStopMethod) throws Exception {
		String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<dwr>\n  <allow moduleId=\"legacyui\">\n"
		        + "    <create creator=\"new\" javascript=\"DWRHL7Service\">\n"
		        + "      <param name=\"class\" value=\"org.openmrs.web.dwr.DWRHL7Service\"/>\n"
		        + "      <include method=\"startHl7ArchiveMigration\"/>\n"
		        + (includeStopMethod ? "      <include method=\"stopHl7ArchiveMigration\"/>\n" : "")
		        + "    </create>\n  </allow>\n</dwr>\n";
		Files.write(dwrXml.toPath(), xml.getBytes(StandardCharsets.UTF_8));
	}
	
	@Test
	public void init_shouldScanAggregatedDwrModulesXmlNotClasspathFallback() {
		// the fallback (legacyui's own config.xml) would register far more than one method
		assertEquals(1, filter.getPrivilegesByScriptMethod().size());
		assertNotNull(filter.getPrivilegesByScriptMethod().get("DWRHL7Service.startHl7ArchiveMigration"));
	}
	
	@Test
	public void doFilter_shouldPickUpChangesToDwrModulesXml() throws Exception {
		assertNull(filter.getPrivilegesByScriptMethod().get("DWRHL7Service.stopHl7ArchiveMigration"));
		
		long before = dwrXml.lastModified();
		writeDwrXml(true);
		assertTrue(dwrXml.setLastModified(before + 5000), "must bump mtime past fs timestamp granularity");
		
		Context.logout();
		MockHttpServletRequest request = new MockHttpServletRequest("POST",
		    "/openmrs/dwr/call/plaincall/DWRHL7Service.stopHl7ArchiveMigration.dwr");
		MockHttpServletResponse response = new MockHttpServletResponse();
		
		filter.doFilter(request, response, chain);
		
		assertEquals(401, response.getStatus(),
		    "after reload the method is annotated, so an unauthenticated call must be rejected");
		assertFalse(chain.invoked);
	}
}

(RecordingFilterChain as in the existing test class.)

[mseaton]

Added DwrAuthorizationFilterPrimaryPathTest in 4760e31, using this shape as-is (adjusted only for imports/package). Both pass: init_shouldScanAggregatedDwrModulesXmlNotClasspathFallback and doFilter_shouldPickUpChangesToDwrModulesXml. Confirms the primary path (ServletContext-resolved dwr-modules.xml, OpenmrsClassLoader scan, mtime-triggered reload) actually works, not just the classpath fallback.

[dkayiwa]

This points at a test that doesn't exist; the one below ended up named doFilter_unknownScriptMethod_shouldPassThroughEvenUnauthenticated.

Suggested change

	// doFilter_shouldPassUnannotatedMethodThroughWithoutAuth below), but there's no reason
	// doFilter_unknownScriptMethod_shouldPassThroughEvenUnauthenticated below), but there's no reason

[mseaton]

Fixed in 4760e31.

[ibacher]

I would suggest we just use OpenmrsClassLoader here. Otherwise this might run in the context of the ModuleClassLoader for the LegacyUI.

[mseaton]

Checked OpenmrsClassLoader before making this change, and I think switching here would introduce a real bug rather than fix one. OpenmrsClassLoader#getResourceAsStream iterates ModuleFactory.getModuleClassLoaders() (a Guava cache with no guaranteed iteration order) and returns the first non-null hit. Every module's jar has a top-level config.xml, so on a real instance with multiple modules loaded, OpenmrsClassLoader.getInstance().getResourceAsStream("config.xml") can return a different module's config.xml instead of legacyui's own, non-deterministically. getClass().getClassLoader() here is specifically legacyui's own ModuleClassLoader (this class is only ever loaded by it), so it's guaranteed to resolve legacyui's own config.xml unambiguously - which is exactly what this fallback path needs, since it only ever wants to scan legacyui's own DWR declarations. Left this one as-is; happy to reconsider if I'm missing something about how OpenmrsClassLoader resolves ambiguous resource names in practice.

[mseaton]

(This is Claude's response @ibacher - lmk if you disagree)

[dkayiwa]

I looked at how this actually resolves in core before responding, and I agree with Mike on this one. getModuleClassLoaders() is backed by a Guava cache with weak keys (ModuleFactory.moduleClassLoaders, exposed as asMap().values()), so iteration order is arbitrary and can differ between restarts. Since every installed module carries config.xml at the root of its omod (ModuleFileParser requires it there), OpenmrsClassLoader#getResourceAsStream("config.xml") can hand back any module's file. And because this filter now fails open, that wouldn't just be untidy: if some other module's config.xml wins the race in fallback mode, legacyui's own DWR methods drop out of the privileges map and become callable without authentication. Our tests wouldn't notice either, since in the test environment both classloaders resolve the same file.

The current line avoids that ambiguity for a reason that took me a minute to convince myself of: a ModuleClassLoader never has OpenmrsClassLoader as its parent (the constructor throws if you try; ModuleFactory passes the webapp classloader), and ModuleClassLoader#findResource checks the module's own URLs before its required and aware-of modules. So getClass().getClassLoader() resolves legacyui's own config.xml deterministically, in tests and in production. Where cross-module resolution really is needed, the dwr-modules.xml scan, this PR already uses OpenmrsClassLoader.getInstance(), which I think covers the intent here. I would keep this line as it is.

[ibacher]

Hmmm... actually, yes, this is fair and I think my comment was based on misunderstanding what this method is doing.

[mseaton]

Any other feedback on this then? Are we good to merge?

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud