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 a1b529169566..90e307dcedf0 100644 --- a/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java +++ b/dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java @@ -179,7 +179,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 a323a6026975..7493638e4e80 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,16 @@ 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 java.nio.charset.StandardCharsets; + +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; import org.dspace.AbstractDSpaceTest; import org.dspace.external.provider.orcid.xml.ExpandedSearchConverter; import org.dspace.utils.DSpace; @@ -34,8 +41,35 @@ 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; + } + + /** + * Build a canned 200 OK ORCID "expanded-search" response for the mock HTTP server, so the cache-aware + * tests below exercise the real Spring {@code @Cacheable} bean without depending on the live ORCID sandbox. + */ + private MockResponse cannedOrcidResponse() throws IOException { + try (InputStream is = cannedResponse(EXPANDED_SEARCH_XML)) { + return new MockResponse() + .setResponseCode(200) + .setHeader("Content-Type", "application/vnd.orcid+xml") + .setBody(new String(is.readAllBytes(), StandardCharsets.UTF_8)); + } + } + @Before public void setup() { sut = new CachingOrcidRestConnector(); @@ -61,40 +95,54 @@ public void getAccessToken() { @Ignore @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()); @@ -105,7 +153,7 @@ public void search_fail() { @Ignore @Test - public void testCachable() { + public void testCachable() throws IOException { CachingOrcidRestConnector c = new DSpace().getServiceManager().getServiceByName( "CachingOrcidRestConnector", CachingOrcidRestConnector.class); @@ -119,44 +167,57 @@ public void testCachable() { verify(c, times(1)).getLabel(orcid); */ - c.setApiURL("https://pub.sandbox.orcid.org/v3.0"); - c.forceAccessToken(sandboxToken); - - String r1 = c.getLabel(orcid); - assertEquals(expectedLabel, r1); - String r2 = c.getLabel(orcid); - assertEquals(expectedLabel, r2); - //get the orcid-labels cache and verify that the label is there - assertEquals(expectedLabel, cache.get(orcid).get()); + // Drive the real Spring @Cacheable bean against a local mock HTTP server instead of the live ORCID + // sandbox, whose dataset is periodically reset and previously caused intermittent CI failures. + try (MockWebServer server = new MockWebServer()) { + // Two responses are enqueued, but with caching working only the FIRST getLabel() hits the server; + // the second is served from the "orcid-labels" cache (asserted via getRequestCount() below). + server.enqueue(cannedOrcidResponse()); + server.enqueue(cannedOrcidResponse()); + + c.setApiURL(server.url("/v3.0").toString()); + c.forceAccessToken(sandboxToken); + + String r1 = c.getLabel(orcid); + assertEquals(expectedLabel, r1); + String r2 = c.getLabel(orcid); + assertEquals(expectedLabel, r2); + //get the orcid-labels cache and verify that the label is there + assertEquals(expectedLabel, cache.get(orcid).get()); + //caching means two getLabel() calls produced a single ORCID API request + assertEquals("Expected getLabel to be cached after the first call", 1, server.getRequestCount()); + } } @Ignore @Test - public void testCacheableWithError() { + public void testCacheableWithError() throws IOException { CachingOrcidRestConnector c = new DSpace().getServiceManager().getServiceByName( "CachingOrcidRestConnector", CachingOrcidRestConnector.class); Cache cache = prepareCache(); assertNull(cache.get(orcid)); - //skip init - c.forceAccessToken(sandboxToken); - //set bad ApiURL to provoke an error - c.setApiURL("https://api.sandbox.orcid.org/"); - String r1 = c.getLabel(orcid); - //on error, getLabel should return null - assertNull(r1); - //the cache should not contain a value for this id - assertNull(cache.get(orcid)); - - //fix the error - c.setApiURL("https://pub.sandbox.orcid.org/v3.0"); - // the error flipped the initialized flag, this reset it - c.forceAccessToken(sandboxToken); - String r2 = c.getLabel(orcid); - assertEquals(expectedLabel, r2); - //the cache should now contain a value for this id - assertEquals(expectedLabel, cache.get(orcid).get()); + try (MockWebServer server = new MockWebServer()) { + //the (mock) ORCID API returns an error first, then a valid response + server.enqueue(new MockResponse().setResponseCode(500)); + server.enqueue(cannedOrcidResponse()); + + //skip init (force a token so getAccessToken/init never reaches out to the network) + c.forceAccessToken(sandboxToken); + c.setApiURL(server.url("/v3.0").toString()); + String r1 = c.getLabel(orcid); + //on error, getLabel should return null + assertNull(r1); + //a null result must NOT be cached (see @Cacheable(unless = "#result == null")) + assertNull(cache.get(orcid)); + + //the second call gets the valid (200) response; the error never cleared the token, so no re-init needed + String r2 = c.getLabel(orcid); + assertEquals(expectedLabel, r2); + //the cache should now contain a value for this id + assertEquals(expectedLabel, cache.get(orcid).get()); + } } private Cache prepareCache() { 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"); diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java index 365e88597d8d..28bbf74a00dd 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java @@ -63,6 +63,7 @@ import org.dspace.eperson.Group; import org.dspace.services.ConfigurationService; import org.hamcrest.Matchers; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -134,6 +135,42 @@ public class AuthorizationRestRepositoryIT extends AbstractControllerIntegration */ private AuthorizationFeature trueForUsersInGroupTest; + private static final String SSR_URL_KEY = "dspace.server.ssr.url"; + private static final String SSR_URL = "http://ssr.example.com/api"; + private static final String ALWAYS_THROW_TURNOFF_KEY = + "org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff"; + + /** + * Configure the SSR object-by-URI resolution used by the {@code search/object} tests (disarm the + * AlwaysThrowExceptionFeature and define the SSR base URL), setting both as JVM system properties + * (+ {@link ConfigurationService#reloadConfig()}) instead of {@code configurationService.setProperty(...)}. + * + *

A plain {@code setProperty(...)} override only lives in the in-memory combined-config view and is + * silently dropped whenever that view is rebuilt by the auto-reload listener (which fires when any + * reloadable cfg file's mtime changes, e.g. another test writing {@code local.cfg}). When that rebuild + * lands mid-test the SSR url disappears and {@code search/object} no longer resolves the uri -> 400 (and a + * dropped turnoff would let {@code alwaysexception} throw -> 500). A system property sits in the + * highest-precedence override layer and is re-read on every rebuild, so it survives auto-reload; it is + * cleared in {@link #clearSsrObjectResolution()}. Same pattern as + * AuthenticationRestControllerIT#setAuthenticationMethodSequence.

+ */ + private void enableSsrObjectResolution() { + System.setProperty(ALWAYS_THROW_TURNOFF_KEY, "true"); + System.setProperty(SSR_URL_KEY, SSR_URL); + configurationService.reloadConfig(); + } + + /** + * Remove the system-property overrides set by {@link #enableSsrObjectResolution()} so they do not leak into + * other tests in the same JVM. Runs before the superclass {@code @After}, whose {@code reloadConfig()} then + * restores the on-disk defaults. + */ + @After + public void clearSsrObjectResolution() { + System.clearProperty(SSR_URL_KEY); + System.clearProperty(ALWAYS_THROW_TURNOFF_KEY); + } + @Override @Before public void setUp() throws Exception { @@ -852,9 +889,9 @@ public void findByObjectSSRTest() throws Exception { SiteRest siteRest = siteConverter.convert(site, DefaultProjection.DEFAULT); String siteUri = "http://ssr.example.com/api/core/sites/" + siteRest.getId(); - // disarm the alwaysThrowExceptionFeature - configurationService.setProperty("org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff", true); - configurationService.setProperty("dspace.server.ssr.url", "http://ssr.example.com/api"); + // Disarm the alwaysThrowExceptionFeature and define the SSR base URL via system properties so the + // overrides survive a mid-test config auto-reload (see enableSsrObjectResolution). + enableSsrObjectResolution(); String adminToken = getAuthToken(admin.getEmail(), password); String epersonToken = getAuthToken(eperson.getEmail(), password); @@ -969,9 +1006,9 @@ public void findByObjectSSRTest() throws Exception { */ @Test public void findByObjectBadRequestSSRTest() throws Exception { - // disarm the alwaysThrowExceptionFeature - configurationService.setProperty("org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff", true); - configurationService.setProperty("dspace.server.ssr.url", "http://ssr.example.com/api"); + // Disarm the alwaysThrowExceptionFeature and define the SSR base URL via system properties so the + // overrides survive a mid-test config auto-reload (see enableSsrObjectResolution). + enableSsrObjectResolution(); String[] invalidUris = new String[] { "invalid-uri", "", @@ -1050,9 +1087,9 @@ public void findByObjectBadRequestSSRTest() throws Exception { public void findByNotExistingObjectSSSTest() throws Exception { String wrongSiteUri = "http://localhost/api/core/sites/" + UUID.randomUUID(); - // disarm the alwaysThrowExceptionFeature - configurationService.setProperty("org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff", true); - configurationService.setProperty("dspace.server.ssr.url", "http://ssr.example.com/api"); + // Disarm the alwaysThrowExceptionFeature and define the SSR base URL via system properties so the + // overrides survive a mid-test config auto-reload (see enableSsrObjectResolution). + enableSsrObjectResolution(); String adminToken = getAuthToken(admin.getEmail(), password); diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java index 63f7e62206f8..7fb2f4cecdf8 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java @@ -7,17 +7,20 @@ */ package org.dspace.app.rest; +import static org.junit.Assert.assertEquals; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; -import java.io.ByteArrayOutputStream; +import java.io.ByteArrayInputStream; import java.io.InputStream; -import java.util.zip.Deflater; +import java.nio.charset.StandardCharsets; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; import org.apache.commons.codec.CharEncoding; -import org.apache.commons.compress.archivers.zip.ZipArchiveEntry; -import org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream; import org.apache.commons.io.IOUtils; import org.dspace.app.rest.model.ItemRest; import org.dspace.app.rest.test.AbstractControllerIntegrationTest; @@ -29,7 +32,6 @@ import org.dspace.content.Bitstream; import org.dspace.content.Collection; import org.dspace.content.Item; -import org.dspace.content.service.BitstreamService; import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -38,6 +40,7 @@ public class MetadataBitstreamControllerIT extends AbstractControllerIntegration private static final String ALL_ZIP_PATH = "allzip"; private static final String HANDLE_PARAM = "handleId"; private static final String AUTHOR = "Test author name"; + private static final String BITSTREAM_CONTENT = "ThisIsSomeDummyText"; private Collection col; private Item publicItem; @@ -46,9 +49,6 @@ public class MetadataBitstreamControllerIT extends AbstractControllerIntegration @Autowired AuthorizeService authorizeService; - @Autowired - BitstreamService bitstreamService; - @Override public void setUp() throws Exception { @@ -64,7 +64,7 @@ public void setUp() throws Exception { .withAuthor(AUTHOR) .build(); - String bitstreamContent = "ThisIsSomeDummyText"; + String bitstreamContent = BITSTREAM_CONTENT; try (InputStream is = IOUtils.toInputStream(bitstreamContent, CharEncoding.UTF_8)) { bts = BitstreamBuilder. createBitstream(context, publicItem, is) @@ -78,23 +78,30 @@ public void setUp() throws Exception { @Test public void downloadAllZip() throws Exception { - ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); - ZipArchiveOutputStream zip = new ZipArchiveOutputStream(byteArrayOutputStream); - zip.setCreateUnicodeExtraFields(ZipArchiveOutputStream.UnicodeExtraFieldPolicy.ALWAYS); - zip.setLevel(Deflater.NO_COMPRESSION); - ZipArchiveEntry ze = new ZipArchiveEntry(bts.getName()); - zip.putArchiveEntry(ze); - InputStream is = bitstreamService.retrieve(context, bts); - org.apache.commons.compress.utils.IOUtils.copy(is, zip); - zip.closeArchiveEntry(); - is.close(); - zip.close(); - String token = getAuthToken(admin.getEmail(), password); - getClient(token).perform(get(METADATABITSTREAM_ENDPOINT + "/" + publicItem.getID() + + byte[] zipBytes = getClient(token).perform(get(METADATABITSTREAM_ENDPOINT + "/" + publicItem.getID() + "/" + ALL_ZIP_PATH).param(HANDLE_PARAM, publicItem.getHandle())) .andExpect(status().isOk()) - .andExpect(content().bytes(byteArrayOutputStream.toByteArray())); + .andReturn().getResponse().getContentAsByteArray(); + + // A ZIP entry stores a DOS last-modified timestamp that defaults to "now" at 2-second resolution, so + // comparing the response byte-for-byte against a locally-built ZIP intermittently failed when the server + // and the test happened to build their entries in different time buckets. Assert the meaningful payload + // instead: the archive must contain exactly the item's bitstream, with the expected content. + Map entries = new HashMap<>(); + int entryCount = 0; + try (ZipInputStream zis = new ZipInputStream(new ByteArrayInputStream(zipBytes))) { + ZipEntry entry; + while ((entry = zis.getNextEntry()) != null) { + entryCount++; + entries.put(entry.getName(), new String(IOUtils.toByteArray(zis), StandardCharsets.UTF_8)); + zis.closeEntry(); + } + } + // count tracked separately so a duplicate entry name can't be masked by the map + assertEquals(1, entryCount); + assertEquals(Set.of(bts.getName()), entries.keySet()); + assertEquals(BITSTREAM_CONTENT, entries.get(bts.getName())); } } diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/StatisticsRestRepositoryIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/StatisticsRestRepositoryIT.java index 6989c208617d..716ccf8b8de6 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/StatisticsRestRepositoryIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/StatisticsRestRepositoryIT.java @@ -938,6 +938,13 @@ public void topCountriesReport_Community_Visited() throws Exception { .contentType(contentType)) .andExpect(status().isCreated()); + // Force a commit that waits for a new searcher so the two just-posted view events are guaranteed to be + // flushed and visible to the report query below. This covers both ways the report could otherwise come + // back empty: postView()'s own commit uses waitSearcher=false (can return before the searcher reopens), + // and if the solr-statistics.autoCommit=false override were dropped by a mid-test config reload then + // postView() skips its commit entirely (Solr's own autoCommit only fires after 10s). + StatisticsServiceFactory.getInstance().getSolrLoggerService().commit(); + // And request that collection's TopCountries report getClient(adminToken).perform( get("/api/statistics/usagereports/" + communityVisited.getID() + "_" + TOP_COUNTRIES_REPORT_ID))