diff --git a/changelog.html b/changelog.html index 2a0aa6a72..e65ec7a4e 100644 --- a/changelog.html +++ b/changelog.html @@ -46,6 +46,7 @@

1.12.1 (to be determined)

diff --git a/src/java/org/jivesoftware/openfire/plugin/rest/controller/SystemController.java b/src/java/org/jivesoftware/openfire/plugin/rest/controller/SystemController.java index d57ddeb87..1729c0da3 100644 --- a/src/java/org/jivesoftware/openfire/plugin/rest/controller/SystemController.java +++ b/src/java/org/jivesoftware/openfire/plugin/rest/controller/SystemController.java @@ -97,7 +97,8 @@ public SystemProperties getSystemProperties() { * @throws ServiceException the service exception */ public org.jivesoftware.openfire.plugin.rest.entity.SystemProperty getSystemProperty(String propertyKey) throws ServiceException { - String propertyValue = JiveGlobals.getProperty(propertyKey); + final Optional systemProperty = SystemProperty.getProperty(propertyKey); + final String propertyValue = systemProperty.isPresent() ? systemProperty.get().getValueAsSaved() : JiveGlobals.getProperty(propertyKey); if(propertyValue != null) { return new org.jivesoftware.openfire.plugin.rest.entity.SystemProperty(propertyKey, propertyValue); } else { diff --git a/src/test/java/org/jivesoftware/openfire/plugin/rest/controller/SystemControllerTest.java b/src/test/java/org/jivesoftware/openfire/plugin/rest/controller/SystemControllerTest.java new file mode 100644 index 000000000..450fed762 --- /dev/null +++ b/src/test/java/org/jivesoftware/openfire/plugin/rest/controller/SystemControllerTest.java @@ -0,0 +1,161 @@ +/* + * Copyright (C) 2026 Ignite Realtime Foundation. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.jivesoftware.openfire.plugin.rest.controller; + +import org.jivesoftware.openfire.plugin.rest.exceptions.ExceptionType; +import org.jivesoftware.openfire.plugin.rest.exceptions.ServiceException; +import org.jivesoftware.util.JiveGlobals; +import org.jivesoftware.util.SystemProperty; +import org.junit.Before; +import org.junit.Test; +import org.mockito.MockedStatic; + +import javax.ws.rs.core.Response; +import java.util.Optional; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.when; + +/** + * Unit tests for {@link SystemController}, in particular for the retrieval of an individual system property. + * + * These tests mock the static {@link SystemProperty} and {@link JiveGlobals} entry points, as neither is usable + * outside of a running Openfire server. + * + * @see issue #242 + */ +public class SystemControllerTest { + + private SystemController systemController; + + @Before + public void setUp() { + // Deliberately not using SystemController.getInstance(): that singleton can have been replaced by a mock + // controller by other (Jersey-level) tests that run in the same JVM, via SystemController#setInstance. + systemController = new SystemController(); + } + + /** + * A property that was registered (by Openfire, or by a plugin) using the {@link SystemProperty} API, but that + * has never been given an explicit value, should be returned using its default value, instead of causing a 404. + * + * This reproduces the bug reported in issue #242, where {@code /system/properties} lists such a property (as it + * queries the {@link SystemProperty} registry, falling back to {@link JiveGlobals}), while + * {@code /system/properties/{propertyKey}} 404'd (as it queried {@link JiveGlobals} only). + */ + @Test + public void testGetSystemPropertyThatIsRegisteredButUnset() throws Exception { + final String key = "foo.bar.xyz"; + + try (final MockedStatic systemPropertyMock = mockStatic(SystemProperty.class)) { + final SystemProperty registeredProperty = mock(SystemProperty.class); + when(registeredProperty.getValueAsSaved()).thenReturn("false"); + systemPropertyMock.when(() -> SystemProperty.getProperty(eq(key))).thenReturn(Optional.of(registeredProperty)); + + final org.jivesoftware.openfire.plugin.rest.entity.SystemProperty result = systemController.getSystemProperty(key); + + assertEquals(key, result.getKey()); + assertEquals("false", result.getValue()); + } + } + + /** + * A property that is registered using the {@link SystemProperty} API and has an explicit value should return + * that value. + */ + @Test + public void testGetSystemPropertyThatIsRegisteredAndSet() throws Exception { + final String key = "foo.bar.xyz"; + + try (final MockedStatic systemPropertyMock = mockStatic(SystemProperty.class)) { + final SystemProperty registeredProperty = mock(SystemProperty.class); + when(registeredProperty.getValueAsSaved()).thenReturn("true"); + systemPropertyMock.when(() -> SystemProperty.getProperty(eq(key))).thenReturn(Optional.of(registeredProperty)); + + final org.jivesoftware.openfire.plugin.rest.entity.SystemProperty result = systemController.getSystemProperty(key); + + assertEquals(key, result.getKey()); + assertEquals("true", result.getValue()); + } + } + + /** + * A property that is not registered using the {@link SystemProperty} API, but that does exist as a plain + * {@link JiveGlobals} property, should still be returned (pre-existing behavior, unaffected by the fix for + * issue #242). + */ + @Test + public void testGetSystemPropertyThatIsOnlyInJiveGlobals() throws Exception { + final String key = "some.unregistered.property"; + + try (final MockedStatic systemPropertyMock = mockStatic(SystemProperty.class); + final MockedStatic jiveGlobalsMock = mockStatic(JiveGlobals.class)) { + systemPropertyMock.when(() -> SystemProperty.getProperty(eq(key))).thenReturn(Optional.empty()); + jiveGlobalsMock.when(() -> JiveGlobals.getProperty(eq(key))).thenReturn("bar"); + + final org.jivesoftware.openfire.plugin.rest.entity.SystemProperty result = systemController.getSystemProperty(key); + + assertEquals(key, result.getKey()); + assertEquals("bar", result.getValue()); + } + } + + /** + * A property that is neither registered using the {@link SystemProperty} API, nor set as a plain + * {@link JiveGlobals} property, should still result in a 404 (pre-existing behavior, unaffected by the fix for + * issue #242). + */ + @Test + public void testGetSystemPropertyThatDoesNotExist() { + final String key = "does.not.exist"; + + try (final MockedStatic systemPropertyMock = mockStatic(SystemProperty.class); + final MockedStatic jiveGlobalsMock = mockStatic(JiveGlobals.class)) { + systemPropertyMock.when(() -> SystemProperty.getProperty(eq(key))).thenReturn(Optional.empty()); + jiveGlobalsMock.when(() -> JiveGlobals.getProperty(eq(key))).thenReturn(null); + + final ServiceException exception = assertThrows(ServiceException.class, () -> systemController.getSystemProperty(key)); + + assertEquals(ExceptionType.PROPERTY_NOT_FOUND, exception.getException()); + assertEquals(Response.Status.NOT_FOUND, exception.getStatus()); + } + } + + /** + * When a property is registered using the {@link SystemProperty} API, its value should be used without + * consulting {@link JiveGlobals} directly (the {@link SystemProperty} API does that internally already). + */ + @Test + public void testGetSystemPropertyDoesNotConsultJiveGlobalsWhenRegistered() throws Exception { + final String key = "foo.bar.xyz"; + + try (final MockedStatic systemPropertyMock = mockStatic(SystemProperty.class); + final MockedStatic jiveGlobalsMock = mockStatic(JiveGlobals.class)) { + final SystemProperty registeredProperty = mock(SystemProperty.class); + when(registeredProperty.getValueAsSaved()).thenReturn("false"); + systemPropertyMock.when(() -> SystemProperty.getProperty(eq(key))).thenReturn(Optional.of(registeredProperty)); + + systemController.getSystemProperty(key); + + jiveGlobalsMock.verify(() -> JiveGlobals.getProperty(eq(key)), never()); + } + } +} diff --git a/src/test/java/org/jivesoftware/openfire/plugin/rest/service/SystemServiceBackwardCompatibilityTest.java b/src/test/java/org/jivesoftware/openfire/plugin/rest/service/SystemServiceBackwardCompatibilityTest.java new file mode 100644 index 000000000..8ee257656 --- /dev/null +++ b/src/test/java/org/jivesoftware/openfire/plugin/rest/service/SystemServiceBackwardCompatibilityTest.java @@ -0,0 +1,212 @@ +/* + * Copyright (C) 2026 Ignite Realtime Foundation. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.jivesoftware.openfire.plugin.rest.service; + +import org.glassfish.hk2.utilities.binding.AbstractBinder; +import org.glassfish.jersey.server.ResourceConfig; +import org.glassfish.jersey.test.JerseyTest; +import org.jivesoftware.openfire.plugin.rest.CustomJacksonMapperProvider; +import org.jivesoftware.openfire.plugin.rest.controller.SystemController; +import org.jivesoftware.openfire.plugin.rest.entity.SystemProperties; +import org.jivesoftware.openfire.plugin.rest.entity.SystemProperty; +import org.jivesoftware.openfire.plugin.rest.exceptions.ExceptionType; +import org.jivesoftware.openfire.plugin.rest.exceptions.RESTExceptionMapper; +import org.jivesoftware.openfire.plugin.rest.exceptions.ServiceException; +import org.junit.BeforeClass; +import org.junit.Test; + +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.client.Entity; +import javax.ws.rs.core.Application; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; +import java.util.Arrays; + +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.mockito.Mockito.withSettings; + +/** + * Asserts that service endpoints in restapi/v1/system have a stable signature. + * + * The tests in this class interact with the REST API as instantiated in this plugin, using a mock service + * controller implementation. Unlike the other *BackwardCompatibilityTest classes in this package, the + * expected values recorded here were not captured from a historic release of this plugin, as none previously existed + * for this service. Instead, they establish the current, stable response shape, so that future changes are made + * deliberately rather than accidentally. + * + * This class also covers the scenario reported in + * issue #242, where + * {@code GET /system/properties/{propertyKey}} could 404 for a property that {@code GET /system/properties} listed. + * The controller-level cause and fix are covered by {@code SystemControllerTest}; here, the fix is confirmed to be + * reachable and correctly mapped to a 404 response through the actual service and exception mapper. + */ +public class SystemServiceBackwardCompatibilityTest extends JerseyTest { + + private static final String EXISTING_KEY = "foo.bar.xyz"; + private static final String EXISTING_VALUE = "false"; + private static final String MISSING_KEY = "does.not.exist"; + + /** + * Constructs the mock of the service controller that mimics the 'business logic' normally provided by a running + * Openfire server. + * + * @return A mock of a SystemController + */ + public static SystemController constructMockController() throws ServiceException { + final SystemController controller = mock(SystemController.class, withSettings().lenient()); + + final SystemProperty one = new SystemProperty(EXISTING_KEY, EXISTING_VALUE); + final SystemProperty two = new SystemProperty("xmpp.domain", "example.org"); + + doAnswer(invocationOnMock -> { + final SystemProperties result = new SystemProperties(); + result.setProperties(Arrays.asList(one, two)); + return result; + }).when(controller).getSystemProperties(); + + doAnswer(invocationOnMock -> one).when(controller).getSystemProperty(eq(EXISTING_KEY)); + doAnswer(invocationOnMock -> { + throw new ServiceException("Could not find property", MISSING_KEY, ExceptionType.PROPERTY_NOT_FOUND, Response.Status.NOT_FOUND); + }).when(controller).getSystemProperty(eq(MISSING_KEY)); + + doAnswer(invocationOnMock -> null).when(controller).createSystemProperty(any()); + doAnswer(invocationOnMock -> null).when(controller).updateSystemProperty(eq(EXISTING_KEY), any()); + doAnswer(invocationOnMock -> null).when(controller).deleteSystemProperty(eq(EXISTING_KEY)); + + return controller; + } + + @BeforeClass + public static void setUpClass() throws ServiceException { + // Override the service controller with a mock controller. + SystemController.setInstance(constructMockController()); + } + + @Override + protected Application configure() { + // Configures the Jersey web application. This should mimic JerseyWrapper's implementation. + final ResourceConfig config = new ResourceConfig(SystemService.class, RESTExceptionMapper.class, CustomJacksonMapperProvider.class); + + // RESTExceptionMapper injects the servlet request via @Context. The test container used here isn't + // servlet-based, so nothing would otherwise supply that binding, causing a NullPointerException from + // the mapper for every non-2xx response. + final HttpServletRequest mockRequest = mock(HttpServletRequest.class, withSettings().lenient()); + when(mockRequest.getMethod()).thenReturn("GET"); + config.register(new AbstractBinder() { + @Override + protected void configure() { + bind(mockRequest).to(HttpServletRequest.class); + } + }); + + return config; + } + + @Test + public void getPropertiesXml() { + Response response = target("restapi/v1/system/properties").request(MediaType.APPLICATION_XML).get(); + + String content = response.readEntity(String.class); + assertEquals("Content of response should match the current, stable response shape.", "", content); + assertEquals("HTTP response should have a status code that is 200.", Response.Status.OK.getStatusCode(), response.getStatus()); + } + + /** + * The JSON-based equivalent of {@link #getPropertiesXml()} + */ + @Test + public void getPropertiesJson() { + Response response = target("restapi/v1/system/properties").request(MediaType.APPLICATION_JSON).get(); + + String content = response.readEntity(String.class); + assertEquals("Content of response should match the current, stable response shape.", "{\"property\":[{\"key\":\"foo.bar.xyz\",\"value\":\"false\"},{\"key\":\"xmpp.domain\",\"value\":\"example.org\"}]}", content); + assertEquals("HTTP response should have a status code that is 200.", Response.Status.OK.getStatusCode(), response.getStatus()); + } + + /** + * Retrieves a property that is known to exist. This is the counterpart of {@link #getPropertyNotFoundXml()}, + * which exercises the 404 that issue #242 reported for a property that does, in fact, exist. + */ + @Test + public void getPropertyXml() { + Response response = target("restapi/v1/system/properties/" + EXISTING_KEY).request(MediaType.APPLICATION_XML).get(); + + String content = response.readEntity(String.class); + assertEquals("Content of response should match the current, stable response shape.", "", content); + assertEquals("HTTP response should have a status code that is 200.", Response.Status.OK.getStatusCode(), response.getStatus()); + } + + /** + * The JSON-based equivalent of {@link #getPropertyXml()} + */ + @Test + public void getPropertyJson() { + Response response = target("restapi/v1/system/properties/" + EXISTING_KEY).request(MediaType.APPLICATION_JSON).get(); + + String content = response.readEntity(String.class); + assertEquals("Content of response should match the current, stable response shape.", "{\"key\":\"foo.bar.xyz\",\"value\":\"false\"}", content); + assertEquals("HTTP response should have a status code that is 200.", Response.Status.OK.getStatusCode(), response.getStatus()); + } + + /** + * Retrieves a property that does not exist, and asserts that this is reported as a 404, rather than the request + * erroring out in some other fashion. + */ + @Test + public void getPropertyNotFoundXml() { + Response response = target("restapi/v1/system/properties/" + MISSING_KEY).request(MediaType.APPLICATION_XML).get(); + + assertEquals("HTTP response should have a status code that is 404.", Response.Status.NOT_FOUND.getStatusCode(), response.getStatus()); + } + + /** + * The JSON-based equivalent of {@link #getPropertyNotFoundXml()} + */ + @Test + public void getPropertyNotFoundJson() { + Response response = target("restapi/v1/system/properties/" + MISSING_KEY).request(MediaType.APPLICATION_JSON).get(); + + assertEquals("HTTP response should have a status code that is 404.", Response.Status.NOT_FOUND.getStatusCode(), response.getStatus()); + } + + @Test + public void createPropertyXml() { + Response response = target("restapi/v1/system/properties").request(MediaType.APPLICATION_XML) + .post(Entity.xml("")); + + assertEquals("HTTP response should have a status code that is 201.", Response.Status.CREATED.getStatusCode(), response.getStatus()); + } + + @Test + public void updatePropertyXml() { + Response response = target("restapi/v1/system/properties/" + EXISTING_KEY).request(MediaType.APPLICATION_XML) + .put(Entity.xml("")); + + assertEquals("HTTP response should have a status code that is 200.", Response.Status.OK.getStatusCode(), response.getStatus()); + } + + @Test + public void deletePropertyXml() { + Response response = target("restapi/v1/system/properties/" + EXISTING_KEY).request(MediaType.APPLICATION_XML).delete(); + + assertEquals("HTTP response should have a status code that is 200.", Response.Status.OK.getStatusCode(), response.getStatus()); + } +} diff --git a/test/system.hurl b/test/system.hurl index 51a8eb62f..44e768256 100644 --- a/test/system.hurl +++ b/test/system.hurl @@ -38,6 +38,16 @@ xpath "/properties/property" count > 150 # TODO test that property with attribute key=admin.authorizedJIDs exists # TODO test that property with attribute key=plugin.restapi.enabled has value=true +# https://github.com/igniterealtime/openfire-restAPI-plugin/issues/242 +# abstractGroupProvider.shared.recursive is registered through Openfire's SystemProperty API with a default value. +# It used to 404 when queried individually, as that lookup checked JiveGlobals only. +GET http://localhost:9090/plugins/restapi/v1/system/properties/abstractGroupProvider.shared.recursive +Authorization: {{authkey}} +HTTP 200 +[Asserts] +xpath "/property" count == 1 +xpath "string(/property/@value)" == "false" + POST http://localhost:9090/plugins/restapi/v1/system/properties Authorization: {{authkey}} Content-Type: application/xml @@ -83,4 +93,4 @@ HTTP 400 DELETE http://localhost:9090/plugins/restapi/v1/system/properties/test.key Authorization: {{authkey}} -HTTP 200 \ No newline at end of file +HTTP 200