diff --git a/dspace-api/src/main/java/org/dspace/core/AbstractHibernateDAO.java b/dspace-api/src/main/java/org/dspace/core/AbstractHibernateDAO.java index 32ad747d765e..498c52c27f46 100644 --- a/dspace-api/src/main/java/org/dspace/core/AbstractHibernateDAO.java +++ b/dspace-api/src/main/java/org/dspace/core/AbstractHibernateDAO.java @@ -306,11 +306,23 @@ public Iterator iterate(Query query) { return new AbstractIterator () { @Override protected T computeNext() { - return iter.hasNext() ? iter.next() : endOfData(); - } - @Override - public void finalize() { + if (iter.hasNext()) { + return iter.next(); + } + // Close the backing ScrollableResults / JDBC cursor as soon as the iteration is exhausted, on + // the thread that owns the Hibernate Session (the caller's thread). + // + // This MUST NOT be done from a finalize() override (as it previously was): finalize() runs on + // the GC Finalizer thread, so closing the stream there mutates the Session's per-session, + // non-thread-safe JDBC ResourceRegistry (xref) concurrently with the owning thread. That is a + // genuine data race which intermittently throws ConcurrentModificationException from + // ResourceRegistryStandardImpl.releaseResources during an unrelated commit/rollback (observed + // as flaky integration-test failures in AbstractIntegrationTestWithDatabase teardown). + // + // An iterator abandoned before exhaustion no longer leaks: its open statement is released + // safely when the owning Context/Session is closed (releaseResources runs on the owning thread). stream.close(); + return endOfData(); } }; } diff --git a/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java b/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java index e34767a25063..4eee572d5a6f 100644 --- a/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java +++ b/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java @@ -174,7 +174,9 @@ protected String getAccessToken(String clientSecret, String clientId, String OAU } } - private InputStream httpGet(String path, String accessToken) throws IOException { + // Package/sub-class visible so tests can stub the HTTP layer (see CachingOrcidRestConnectorTest) + // and avoid hitting the live ORCID sandbox. + protected InputStream httpGet(String path, String accessToken) throws IOException { String trimmedPath = path.replaceFirst("^/+", "").replaceFirst("/+$", ""); String fullPath = apiURL + '/' + trimmedPath; diff --git a/dspace-api/src/test/java/org/dspace/core/AbstractHibernateDAOIteratorIT.java b/dspace-api/src/test/java/org/dspace/core/AbstractHibernateDAOIteratorIT.java new file mode 100644 index 000000000000..fcc00434d620 --- /dev/null +++ b/dspace-api/src/test/java/org/dspace/core/AbstractHibernateDAOIteratorIT.java @@ -0,0 +1,76 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.core; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; + +import java.util.Iterator; + +import org.dspace.AbstractIntegrationTestWithDatabase; +import org.dspace.content.MetadataValue; +import org.dspace.content.factory.ContentServiceFactory; +import org.dspace.content.service.MetadataValueService; +import org.junit.Test; + +/** + * Regression test for the intermittent {@code ConcurrentModificationException} thrown during + * {@code @After} integration-test cleanup and traced to + * {@link AbstractHibernateDAO#iterate(javax.persistence.Query)}. + * + *

That method used to close its Hibernate {@code Stream} from a {@code finalize()} override. {@code finalize()} + * runs on the GC Finalizer thread, so closing the stream there mutated the owning {@code Session}'s per-session, + * non-thread-safe JDBC {@code ResourceRegistry} (xref) concurrently with the thread that owns the session. That + * is a genuine data race which intermittently threw {@code ConcurrentModificationException} from + * {@code ResourceRegistryStandardImpl.releaseResources} during an unrelated commit/rollback. The fix closes the + * stream on the owning thread once the iteration is exhausted. This test guards against reintroducing any + * stream-closing finalizer on the returned iterator.

+ */ +public class AbstractHibernateDAOIteratorIT extends AbstractIntegrationTestWithDatabase { + + private final MetadataValueService metadataValueService = + ContentServiceFactory.getInstance().getMetadataValueService(); + + /** + * Verifies that the iterator returned by {@link AbstractHibernateDAO#iterate(javax.persistence.Query)} + * (exercised here through {@code MetadataValueService.findByValueLike}) does not close its backing Hibernate + * stream from a {@code finalize()} override anywhere in its class hierarchy, and that it still iterates to + * exhaustion (closing its cursor on the owning thread) without error. No matching rows are required - the + * wrapper iterator is created regardless of the result count. + * + * @throws Exception passed through. + */ + @Test + public void iterateIteratorMustNotCloseStreamFromFinalizer() throws Exception { + Iterator iterator = + metadataValueService.findByValueLike(context, "no-such-metadata-value-" + System.nanoTime()); + assertNotNull(iterator); + + // The returned iterator - and every class in its hierarchy up to Object - MUST NOT declare a finalize() + // override: closing the backing Hibernate Stream from the GC Finalizer thread is exactly the cross-thread + // access to the non-thread-safe per-session JDBC ResourceRegistry that caused the flaky + // ConcurrentModificationException. Walking the hierarchy also catches a finalizer reintroduced on a + // superclass/helper rather than on the anonymous leaf class. + for (Class type = iterator.getClass(); type != null && type != Object.class; type = type.getSuperclass()) { + try { + type.getDeclaredMethod("finalize"); + fail("AbstractHibernateDAO.iterate() iterator must not declare a finalize() override (found on " + + type.getName() + ") - closing the Hibernate Stream on the GC Finalizer thread races the " + + "owning thread's non-thread-safe JDBC ResourceRegistry and intermittently throws " + + "ConcurrentModificationException."); + } catch (NoSuchMethodException expected) { + // good: no stream-closing finalizer on this class + } + } + + // It must still iterate to exhaustion and close its cursor on THIS (the owning) thread without error. + while (iterator.hasNext()) { + assertNotNull(iterator.next()); + } + } +} diff --git a/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java b/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java index bdb051601cb8..7e8cbc6c94fc 100644 --- a/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java +++ b/dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java @@ -13,9 +13,13 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import java.io.IOException; +import java.io.InputStream; + import org.dspace.AbstractDSpaceTest; import org.dspace.external.provider.orcid.xml.ExpandedSearchConverter; import org.dspace.utils.DSpace; @@ -33,8 +37,22 @@ public class CachingOrcidRestConnectorTest extends AbstractDSpaceTest { private static final String orcid = "0000-0002-9150-2529"; private static final String expectedLabel = "Connor, John"; + // Canned ORCID "expanded-search" response (num-found=1725, first result -> "Connor, John"). + // Used to mock the HTTP layer so the tests don't depend on the live ORCID sandbox. + private static final String EXPANDED_SEARCH_XML = "org/dspace/external/orcid-expanded-search.xml"; + private CachingOrcidRestConnector sut; + /** + * Load a canned API response from the test classpath as a fresh InputStream. + * (A new stream is returned on every call because the connector consumes/closes it.) + */ + private InputStream cannedResponse(String resource) { + InputStream is = getClass().getClassLoader().getResourceAsStream(resource); + assertNotNull("Missing test resource: " + resource, is); + return is; + } + @Before public void setup() { sut = new CachingOrcidRestConnector(); @@ -59,40 +77,54 @@ public void getAccessToken() { } @Test - public void getLabel() { + public void getLabel() throws Exception { sut = Mockito.spy(sut); sut.setApiURL("https://pub.sandbox.orcid.org/v3.0"); //Mock the CachingOrcidRestConnector so that getAccessToken returns sandboxToken doReturn(sandboxToken).when(sut).getAccessToken(Mockito.anyString(), Mockito.anyString(), Mockito.anyString()); + //Mock the HTTP layer with a canned response so we don't depend on the live ORCID sandbox. + doReturn(cannedResponse(EXPANDED_SEARCH_XML)).when(sut).httpGet(Mockito.anyString(), Mockito.anyString()); String label = sut.getLabel(orcid); assertEquals(expectedLabel, label); } @Test - public void search() { + public void search() throws Exception { sut = Mockito.spy(sut); sut.setApiURL("https://pub.sandbox.orcid.org/v3.0"); //Mock the CachingOrcidRestConnector so that getAccessToken returns sandboxToken doReturn(sandboxToken).when(sut).getAccessToken(Mockito.anyString(), Mockito.anyString(), Mockito.anyString()); + //Mock the HTTP layer with a canned ORCID expanded-search response. Previously this test hit the live + //ORCID sandbox and asserted numFound() > 1000, which flaked whenever the sandbox dataset was reset/shrunk. + //Mocking the transport keeps the real parsing + edismax wildcard query-building path under test, but makes + //the result deterministic. + doReturn(cannedResponse(EXPANDED_SEARCH_XML)).when(sut).httpGet(Mockito.anyString(), Mockito.anyString()); ExpandedSearchConverter.Results search = sut.search("joh", 0, 1); - //Should match all Johns also, because edismax with wildcard - assertTrue(search.numFound() > 1000); + assertTrue("Expected a successful ORCID response, got: " + search, search.isOk()); + //'joh' is alphabetic, so the connector turns it into an edismax wildcard query ("joh || joh*") that matches + //many authors; the canned response carries num-found=1725. + assertEquals("Unexpected num-found for the canned ORCID response", 1725L, (long) search.numFound()); + assertEquals("Connor, John", search.results().get(0).label()); } @Test - public void search_fail() { + public void search_fail() throws Exception { sut = Mockito.spy(sut); sut.setApiURL("https://pub.sandbox.orcid.org/v3.0"); - //Mock the CachingOrcidRestConnector so that getAccessToken returns and invalid token + //Mock the CachingOrcidRestConnector so that getAccessToken returns an invalid token doReturn("FAKE").when(sut).getAccessToken(Mockito.anyString(), Mockito.anyString(), Mockito.anyString()); + //Simulate the ORCID API rejecting the (fake) token: every httpGet fails. Done via the mocked HTTP layer + //so the test is deterministic and doesn't rely on the live sandbox returning a 401. + doThrow(new IOException("simulated ORCID auth failure")).when(sut) + .httpGet(Mockito.anyString(), Mockito.anyString()); ExpandedSearchConverter.Results search = sut.search("joh", 0, 1); assertFalse(search.isOk()); - //Further calls fail too, token is stored + //Further calls fail too, token is stored (so getAccessToken is only resolved once) search = sut.search("joh", 0, 1); assertFalse(search.isOk()); diff --git a/dspace-api/src/test/resources/org/dspace/external/orcid-expanded-search.xml b/dspace-api/src/test/resources/org/dspace/external/orcid-expanded-search.xml new file mode 100644 index 000000000000..582731a42038 --- /dev/null +++ b/dspace-api/src/test/resources/org/dspace/external/orcid-expanded-search.xml @@ -0,0 +1,22 @@ + + + + + 0000-0002-9150-2529 + John + Connor + + + 0000-0002-1208-2352 + John + Kendrew + John Kendrew + + diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java index 63318926d752..b23811f27f17 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java @@ -71,6 +71,7 @@ import org.dspace.orcid.model.OrcidTokenResponseDTO; import org.dspace.services.ConfigurationService; import org.hamcrest.Matchers; +import org.junit.After; import org.junit.Before; import org.junit.Ignore; import org.junit.Test; @@ -131,6 +132,45 @@ public class AuthenticationRestControllerIT extends AbstractControllerIntegratio private final String feature = CanChangePasswordFeature.NAME; + /** + * Configuration key for the ordered list of active AuthenticationMethod plugins. + */ + private static final String AUTH_PLUGIN_KEY = + "plugin.sequence.org.dspace.authenticate.AuthenticationMethod"; + + /** + * Replace the active AuthenticationMethod plugin sequence. + * + *

This sets the sequence via a JVM system property (plus an explicit + * {@link org.dspace.services.ConfigurationService#reloadConfig()} so the change is visible + * immediately) rather than via {@link org.dspace.services.ConfigurationService#setProperty(String, Object)}.

+ * + *

A plain {@code setProperty(...)} override only lives in the in-memory view of the combined + * configuration and is silently discarded whenever that view is rebuilt. The auto-reload listener + * rebuilds it as soon as any reloadable cfg file's last-modified timestamp changes (which happens + * intermittently during a CI run, e.g. another test writing {@code local.cfg}). When that rebuild lands + * between this call and the request under test, the on-disk default returns -- in CLARIN that default is + * {@code [PasswordAuthentication, ClarinShibAuthentication]} -- and a stray {@code password realm} leaks + * into the {@code WWW-Authenticate} header even though only e.g. Shibboleth was requested. A system + * property sits in the highest-precedence (override) section of the combined config and is re-read on + * every rebuild, so it survives auto-reload. It is cleared again in + * {@link #clearAuthenticationMethodSequence()}.

+ */ + private void setAuthenticationMethodSequence(String[] methods) { + System.setProperty(AUTH_PLUGIN_KEY, String.join(",", methods)); + configurationService.reloadConfig(); + } + + /** + * Remove the system-property override set by {@link #setAuthenticationMethodSequence(String[])} so it + * does not leak into other test classes running in the same JVM. Runs before the superclass @After, + * whose {@code reloadConfig()} then restores the on-disk default. + */ + @After + public void clearAuthenticationMethodSequence() { + System.clearProperty(AUTH_PLUGIN_KEY); + } + @Before public void setup() throws Exception { super.setUp(); @@ -140,7 +180,7 @@ public void setup() throws Exception { authorization = new Authorization(eperson, canChangePasswordFeature, ePersonRest); // Default all tests to Password Authentication only - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); + setAuthenticationMethodSequence(PASS_ONLY); } @Test @@ -198,7 +238,7 @@ public void testStatusGetSpecialGroups() throws Exception { .withName("specialGroupIP") .build(); - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_AND_IP); + setAuthenticationMethodSequence(PASS_AND_IP); configurationService.setProperty("authentication-password.login.specialgroup","specialGroupPwd"); configurationService.setProperty("authentication-ip.specialGroupIP", "123.123.123.123"); context.restoreAuthSystemState(); @@ -338,7 +378,7 @@ public void testStatusNotAuthenticated() throws Exception { // @Test // public void testStatusShibAuthenticatedWithCookie() throws Exception { // //Enable Shibboleth login only -// configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); +// setAuthenticationMethodSequence(SHIB_ONLY); // // String uiURL = configurationService.getProperty("dspace.ui.url"); // @@ -458,7 +498,7 @@ public void testStatusNotAuthenticated() throws Exception { // @Test // public void testShibbolethEndpointCannotBeUsedWithShibDisabled() throws Exception { // // Enable only password login -// configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); +// setAuthenticationMethodSequence(PASS_ONLY); // // String uiURL = configurationService.getProperty("dspace.ui.url"); // @@ -977,7 +1017,7 @@ public void testLoginGetRequest() throws Exception { public void testShibbolethLoginURLWithDefaultLazyURL() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); //Create a reviewers group Group reviewersGroup = GroupBuilder.createGroup(context) @@ -1001,7 +1041,7 @@ public void testShibbolethLoginURLWithDefaultLazyURL() throws Exception { public void testShibbolethLoginURLWithServerURLContainingPort() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); configurationService.setProperty("dspace.server.url", "http://localhost:8080/server"); configurationService.setProperty("authentication-shibboleth.lazysession.secure", false); @@ -1027,7 +1067,7 @@ public void testShibbolethLoginURLWithServerURLContainingPort() throws Exception public void testShibbolethLoginURLWithConfiguredLazyURL() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); configurationService.setProperty("authentication-shibboleth.lazysession.loginurl", "http://shibboleth.org/Shibboleth.sso/Login"); @@ -1053,7 +1093,7 @@ public void testShibbolethLoginURLWithConfiguredLazyURL() throws Exception { public void testShibbolethLoginURLWithConfiguredLazyURLWithPort() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); configurationService.setProperty("authentication-shibboleth.lazysession.loginurl", "http://shibboleth.org:8080/Shibboleth.sso/Login"); @@ -1081,7 +1121,7 @@ public void testShibbolethLoginURLWithConfiguredLazyURLWithPort() throws Excepti public void testShibbolethLoginRequestAttribute() throws Exception { context.turnOffAuthorisationSystem(); //Enable Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); //Create a reviewers group Group reviewersGroup = GroupBuilder.createGroup(context) @@ -1137,7 +1177,7 @@ public void testShibbolethLoginRequestAttribute() throws Exception { @Ignore // Ignored until an endpoint is added to return all groups public void testShibbolethLoginRequestHeaderWithIpAuthentication() throws Exception { - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_AND_IP); + setAuthenticationMethodSequence(SHIB_AND_IP); configurationService.setProperty("authentication-ip.Administrator", "123.123.123.123"); @@ -1210,7 +1250,7 @@ public void testShibbolethLoginRequestHeaderWithIpAuthentication() throws Except @Test public void testShibbolethAndPasswordAuthentication() throws Exception { //Enable Shibboleth and password login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_AND_PASS); + setAuthenticationMethodSequence(SHIB_AND_PASS); //Check if WWW-Authenticate header contains shibboleth and password getClient().perform(get("/api/authn/status").header("Referer", "http://my.uni.edu")) @@ -1281,7 +1321,7 @@ public void testShibbolethAndPasswordAuthentication() throws Exception { @Test public void testOnlyPasswordAuthenticationWorks() throws Exception { //Enable only password login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); + setAuthenticationMethodSequence(PASS_ONLY); //Check if WWW-Authenticate header contains only getClient().perform(get("/api/authn/status").header("Referer", "http://my.uni.edu")) @@ -1314,7 +1354,7 @@ public void testOnlyPasswordAuthenticationWorks() throws Exception { @Test public void testShibbolethAuthenticationDoesNotWorkWithPassOnly() throws Exception { //Enable only password login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); + setAuthenticationMethodSequence(PASS_ONLY); //Check if WWW-Authenticate header contains only password getClient().perform(get("/api/authn/status").header("Referer", "http://my.uni.edu")) @@ -1332,7 +1372,7 @@ public void testShibbolethAuthenticationDoesNotWorkWithPassOnly() throws Excepti @Test public void testOnlyShibbolethAuthenticationWorks() throws Exception { //Enable only Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); //Check if WWW-Authenticate header contains only shibboleth getClient().perform(get("/api/authn/status").header("Referer", "http://my.uni.edu")) @@ -1365,7 +1405,7 @@ public void testOnlyShibbolethAuthenticationWorks() throws Exception { @Test public void testPasswordAuthenticationDoesNotWorkWithShibOnly() throws Exception { //Enable only Shibboleth login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + setAuthenticationMethodSequence(SHIB_ONLY); getClient().perform(post("/api/authn/login") .param("user", eperson.getEmail()) @@ -1540,7 +1580,7 @@ public void testGenerateShortLivedTokenWithShortLivedToken() throws Exception { // @Test // public void testStatusOrcidAuthenticatedWithCookie() throws Exception { // -// configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", ORCID_ONLY); +// setAuthenticationMethodSequence(ORCID_ONLY); // // String uiURL = configurationService.getProperty("dspace.ui.url"); // @@ -1627,7 +1667,7 @@ public void testGenerateShortLivedTokenWithShortLivedToken() throws Exception { @Test public void testOrcidLoginURL() throws Exception { - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", ORCID_ONLY); + setAuthenticationMethodSequence(ORCID_ONLY); String originalClientId = orcidConfiguration.getClientId(); orcidConfiguration.setClientId("CLIENT-ID"); @@ -1658,7 +1698,7 @@ public void testAreSpecialGroupsApplicable() throws Exception { .withName("specialGroupShib") .build(); - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_AND_PASS); + setAuthenticationMethodSequence(SHIB_AND_PASS); configurationService.setProperty("authentication-password.login.specialgroup", "specialGroupPwd"); configurationService.setProperty("authentication-shibboleth.role.faculty", "specialGroupShib"); configurationService.setProperty("authentication-shibboleth.default-roles", "faculty");