From da8d953d5b442237375dca93de263e63cc963460 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 20 Mar 2024 17:45:11 +0530 Subject: [PATCH 01/20] TRUNK-6203: Global properties access should be privileged --- .../openmrs/api/AdministrationService.java | 9 ++++ .../java/org/openmrs/util/LocaleUtility.java | 4 ++ .../annotation/StartModuleAnnotationTest.java | 8 +-- .../openmrs/aop/AuthorizationAdviceTest.java | 2 +- .../api/AdministrationServiceTest.java | 41 +++++++++------- .../java/org/openmrs/api/UserServiceTest.java | 49 +++++++++++++++---- .../org/openmrs/api/db/ContextDAOTest.java | 23 ++++++++- .../test/StartModuleExecutionListener.java | 6 +++ .../jupiter/StartModuleExecutionListener.java | 6 +++ ...nistrationServiceTest-globalproperties.xml | 4 +- .../org/openmrs/web/filter/GZIPFilter.java | 10 +++- 11 files changed, 125 insertions(+), 37 deletions(-) diff --git a/api/src/main/java/org/openmrs/api/AdministrationService.java b/api/src/main/java/org/openmrs/api/AdministrationService.java index de1466f38e8d..f25d2c7c1e9c 100644 --- a/api/src/main/java/org/openmrs/api/AdministrationService.java +++ b/api/src/main/java/org/openmrs/api/AdministrationService.java @@ -56,6 +56,7 @@ public interface AdministrationService extends OpenmrsService { * Should find object given valid uuid * Should return null if no object found with given uuid */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public GlobalProperty getGlobalPropertyByUuid(String uuid); /** @@ -90,6 +91,7 @@ public interface AdministrationService extends OpenmrsService { * Should get property value given valid property name * Should get property in case insensitive way */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public String getGlobalProperty(String propertyName); /** @@ -106,6 +108,7 @@ public interface AdministrationService extends OpenmrsService { * Should return default value if property name does not exist * Should not fail with null default value */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public String getGlobalProperty(String propertyName, String defaultValue); /** @@ -115,6 +118,7 @@ public interface AdministrationService extends OpenmrsService { * @return the global property that matches the given propertyName * Should return null when no global property match given property name */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public GlobalProperty getGlobalPropertyObject(String propertyName); /** @@ -125,6 +129,7 @@ public interface AdministrationService extends OpenmrsService { * @since 1.5 * Should return all relevant global properties in the database */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public List getGlobalPropertiesByPrefix(String prefix); /** @@ -135,6 +140,7 @@ public interface AdministrationService extends OpenmrsService { * @since 1.6 * Should return all relevant global properties in the database */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public List getGlobalPropertiesBySuffix(String suffix); /** @@ -189,6 +195,7 @@ public interface AdministrationService extends OpenmrsService { * Should overwrite global property if exists * Should save a global property whose typed value is handled by a custom datatype */ + @Authorized(PrivilegeConstants.MANAGE_GLOBAL_PROPERTIES) public void setGlobalProperty(String propertyName, String propertyValue); /** @@ -202,6 +209,7 @@ public interface AdministrationService extends OpenmrsService { * Should fail if global property being updated does not already exist * Should update a global property whose typed value is handled by a custom datatype */ + @Authorized(PrivilegeConstants.MANAGE_GLOBAL_PROPERTIES) public void updateGlobalProperty(String propertyName, String propertyValue); /** @@ -313,6 +321,7 @@ public interface AdministrationService extends OpenmrsService { * @return property value in the type of the default value * @since 1.7 */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public T getGlobalPropertyValue(String propertyName, T defaultValue); /** diff --git a/api/src/main/java/org/openmrs/util/LocaleUtility.java b/api/src/main/java/org/openmrs/util/LocaleUtility.java index 547c1e0384d4..7fb00b43a6d7 100644 --- a/api/src/main/java/org/openmrs/util/LocaleUtility.java +++ b/api/src/main/java/org/openmrs/util/LocaleUtility.java @@ -57,6 +57,7 @@ public static Locale getDefaultLocale() { if (defaultLocaleCache == null) { if (Context.isSessionOpen()) { try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); String locale = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_DEFAULT_LOCALE); @@ -74,6 +75,9 @@ public static Locale getDefaultLocale() { log.warn("Unable to get locale global property value. " + e.getMessage()); log.trace("Unable to get locale global property value", e); } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } // if we weren't able to load the locale from the global property, // use the default one diff --git a/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java b/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java index 573149df7ebc..da5587f1b517 100644 --- a/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java +++ b/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java @@ -9,12 +9,14 @@ */ package org.openmrs.annotation; -import static org.junit.jupiter.api.Assertions.assertNotNull; - import org.junit.jupiter.api.Test; +import org.openmrs.Role; import org.openmrs.api.context.Context; -import org.openmrs.test.jupiter.BaseContextSensitiveTest; import org.openmrs.test.StartModule; +import org.openmrs.test.jupiter.BaseContextSensitiveTest; +import org.openmrs.util.RoleConstants; + +import static org.junit.jupiter.api.Assertions.assertNotNull; @StartModule({ "org/openmrs/module/include/test1-1.0-SNAPSHOT.omod", "org/openmrs/module/include/test2-1.0-SNAPSHOT.omod" }) public class StartModuleAnnotationTest extends BaseContextSensitiveTest { diff --git a/api/src/test/java/org/openmrs/aop/AuthorizationAdviceTest.java b/api/src/test/java/org/openmrs/aop/AuthorizationAdviceTest.java index 851de73e36bf..b60798c05503 100644 --- a/api/src/test/java/org/openmrs/aop/AuthorizationAdviceTest.java +++ b/api/src/test/java/org/openmrs/aop/AuthorizationAdviceTest.java @@ -65,7 +65,7 @@ public void before_shouldNotifyListenersAboutCheckedPrivileges() { Context.getConceptService().saveConcept(concept); String[] privileges = { PrivilegeConstants.MANAGE_CONCEPTS, PrivilegeConstants.GET_OBS, - PrivilegeConstants.GET_CONCEPT_ATTRIBUTE_TYPES }; + PrivilegeConstants.GET_CONCEPT_ATTRIBUTE_TYPES, PrivilegeConstants.GET_GLOBAL_PROPERTIES }; assertThat("listener1", listener1.hasPrivileges, containsInAnyOrder(privileges)); assertThat("listener2", listener2.hasPrivileges, containsInAnyOrder(privileges)); assertThat(listener1.lacksPrivileges, empty()); diff --git a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java index 27c59f3f63ac..081d38ef218b 100644 --- a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java +++ b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java @@ -554,7 +554,7 @@ public void filterGlobalPropertiesByViewPrivilege_shouldFilterGlobalPropertiesIf GlobalProperty property = new GlobalProperty(); property.setProperty("test_property"); property.setPropertyValue("test_property_value"); - property.setViewPrivilege(Context.getUserService().getPrivilege("Some Privilege For View Global Properties")); + property.setViewPrivilege(Context.getUserService().getPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES)); adminService.saveGlobalProperty(property); // assert new test global property is saved properly List properties = adminService.getAllGlobalProperties(); @@ -564,16 +564,19 @@ public void filterGlobalPropertiesByViewPrivilege_shouldFilterGlobalPropertiesIf Context.logout(); Context.authenticate(getTestUserCredentials()); // have to add privilege in order to be able to call getAllGlobalProperties() method for new user - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - properties = adminService.getAllGlobalProperties(); - int actualSize = properties.size(); - - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - Context.logout(); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + properties = adminService.getAllGlobalProperties(); + int actualSize = properties.size(); + Context.logout(); + assertEquals(actualSize, originalSize); + assertTrue(!properties.contains(property)); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } - assertEquals(actualSize, originalSize); - assertTrue(!properties.contains(property)); } /** @@ -588,9 +591,8 @@ public void getGlobalProperty_shouldFailIfUserHasNoPrivileges() { Context.logout(); Context.authenticate(getTestUserCredentials()); - APIException exception = assertThrows(APIException.class, () -> adminService.getGlobalProperty(property.getProperty())); - assertEquals(exception.getMessage(), String.format("Privilege: %s, required to view globalProperty: %s", - property.getViewPrivilege(), property.getProperty())); + APIAuthenticationException exception = assertThrows(APIAuthenticationException.class, () -> adminService.getGlobalProperty(property.getProperty())); + assertEquals(exception.getMessage(), String.format("Privileges required: %s", property.getViewPrivilege())); } /** @@ -608,7 +610,6 @@ public void getGlobalProperty_shouldReturnGlobalPropertyIfUserIsAllowedToView() Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getViewPrivilege()); Context.getAuthenticatedUser().addRole(role); - assertNotNull(adminService.getGlobalProperty(property.getProperty())); } @@ -625,8 +626,8 @@ public void getGlobalPropertyObject_shouldFailIfUserHasNoPrivileges() { Context.authenticate(getTestUserCredentials()); APIException exception = assertThrows(APIException.class, () -> adminService.getGlobalPropertyObject(property.getProperty())); - assertEquals(exception.getMessage(), String.format("Privilege: %s, required to view globalProperty: %s", - property.getViewPrivilege(), property.getProperty())); + assertEquals(exception.getMessage(), String.format("Privileges required: %s", + property.getViewPrivilege())); } /** @@ -662,8 +663,8 @@ public void updateGlobalProperty_shouldFailIfUserIsNotAllowedToEditGlobalPropert Context.authenticate(getTestUserCredentials()); APIException exception = assertThrows(APIException.class, () -> adminService.updateGlobalProperty(property.getProperty(), "new-value")); - assertEquals(exception.getMessage(), String.format("Privilege: %s, required to edit globalProperty: %s", - property.getEditPrivilege(), property.getProperty())); + assertEquals(exception.getMessage(), String.format("Privileges required: %s", + property.getEditPrivilege())); } /** @@ -673,6 +674,7 @@ public void updateGlobalProperty_shouldFailIfUserIsNotAllowedToEditGlobalPropert public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty() { executeDataSet(ADMIN_INITIAL_DATA_XML); GlobalProperty property = getGlobalPropertyWithEditPrivilege(); + GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); assertEquals("anothervalue", property.getPropertyValue()); // authenticate new user without privileges @@ -681,6 +683,7 @@ public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty // add required privilege to user Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getEditPrivilege()); + role.addPrivilege(globalPropertyWithViewPrivilege.getViewPrivilege()); Context.getAuthenticatedUser().addRole(role); adminService.updateGlobalProperty(property.getProperty(), "new-value"); @@ -735,7 +738,7 @@ private GlobalProperty getGlobalPropertyWithViewPrivilege() { GlobalProperty property = adminService.getGlobalPropertyObject("another-global-property"); assertNotNull(property); - Privilege viewPrivilege = Context.getUserService().getPrivilege("Some Privilege For View Global Properties"); + Privilege viewPrivilege = Context.getUserService().getPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); property.setViewPrivilege(viewPrivilege); property = adminService.saveGlobalProperty(property); assertNotNull(property.getViewPrivilege()); @@ -752,7 +755,7 @@ private GlobalProperty getGlobalPropertyWithEditPrivilege() { GlobalProperty property = adminService.getGlobalPropertyObject("another-global-property"); assertNotNull(property); - Privilege editPrivilege = Context.getUserService().getPrivilege("Some Privilege For Edit Global Properties"); + Privilege editPrivilege = Context.getUserService().getPrivilege(PrivilegeConstants.MANAGE_GLOBAL_PROPERTIES); property.setEditPrivilege(editPrivilege); property = adminService.saveGlobalProperty(property); assertNotNull(property.getEditPrivilege()); diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index af2f6b6324dd..85fdfe06e8f1 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -260,6 +260,7 @@ public void createUser_shouldNotAllowCreatingUserWithPrivilegeCurrentUserDoesNot Role userRole = new Role("User Adder"); userRole.setRole(RoleConstants.AUTHENTICATED); userRole.addPrivilege(new Privilege("Add Users")); + userRole.addPrivilege(new Privilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES)); currentUser.addRole(userRole); // setup our expected exception // we expect this to fail because the currently logged-in user lacks a privilege to be @@ -293,6 +294,7 @@ public void createUser_shouldNotAllowCreatingUserWithPrivilegesCurrentUserDoesNo Role userRole = new Role("User Adder"); userRole.setRole(RoleConstants.AUTHENTICATED); userRole.addPrivilege(new Privilege("Add Users")); + userRole.addPrivilege(new Privilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES)); currentUser.addRole(userRole); // setup our expected exception // we expect this to fail because the currently logged-in user lacks a privilege to be @@ -327,8 +329,8 @@ public void createUser_shouldNotAllowAssigningSuperUserRoleIfCurrentUserDoesNotH Role userRole = new Role("User Adder"); userRole.setRole(RoleConstants.AUTHENTICATED); userRole.addPrivilege(new Privilege("Add Users")); + userRole.addPrivilege(new Privilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES)); currentUser.addRole(userRole); - // setup our expected exception // we expect this to fail because the currently logged-in user lacks a privilege to be // assigned to the new user @@ -494,8 +496,14 @@ public void changePassword_shouldMatchOnIncorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("incorrectlyhashedSha1", "test"); - - userService.changePassword("test", "Tester12"); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } + Context.logout(); // so that the next test reauthenticates } @@ -536,8 +544,13 @@ public void changePassword_shouldMatchOnCorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("correctlyhashedSha1", "test"); - - userService.changePassword("test", "Tester12"); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } Context.logout(); // so that the next test reauthenticates } @@ -564,9 +577,13 @@ public void changePassword_shouldMatchOnSha512HashedPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("userWithSha512Hash", "test"); - - userService.changePassword("test", "Tester12"); - + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } Context.logout(); // so that the next test reauthenticates } @@ -1512,8 +1529,14 @@ public void changePasswordUsingSecretAnswer_shouldUpdatePasswordIfSecretIsCorrec User user = userService.getUser(6001); assertFalse(user.hasPrivilege(PrivilegeConstants.EDIT_USER_PASSWORDS)); Context.authenticate(user.getUsername(), "userServiceTest"); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePasswordUsingSecretAnswer("answer", "userServiceTest2"); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } - userService.changePasswordUsingSecretAnswer("answer", "userServiceTest2"); Context.authenticate(user.getUsername(), "userServiceTest2"); } @@ -1606,7 +1629,13 @@ public void changePasswordUsingActivationKey_shouldUpdatePasswordIfActivationKey final String PASSWORD = "Admin123"; Context.authenticate(createdUser.getUsername(), "Openmr5xy"); - userService.changePasswordUsingActivationKey(key, PASSWORD); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePasswordUsingActivationKey(key, PASSWORD); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } Context.authenticate(createdUser.getUsername(), PASSWORD); } diff --git a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java index 688c5cfbf3f3..7e6525934253 100644 --- a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java +++ b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java @@ -32,6 +32,7 @@ import org.openmrs.api.context.ContextAuthenticationException; import org.openmrs.api.db.hibernate.HibernateContextDAO; import org.openmrs.test.jupiter.BaseContextSensitiveTest; +import org.openmrs.util.PrivilegeConstants; import org.springframework.stereotype.Component; /** @@ -278,38 +279,58 @@ public void authenticate_shouldPassRegressionTestFor1580() { // logout after the base setup Context.logout(); + // first we fail a login attempt try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "not the right password"); fail("Not sure why this username/password combo worked"); } catch (ContextAuthenticationException authException) { // pass } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } // next we log in correctly try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "test"); } catch (ContextAuthenticationException authException) { fail("There must be an admin:test user for this test to run properly"); } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } Context.logout(); for (int x = 1; x <= 8; x++) { // now we fail several login attempts try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "not the right password"); fail("Not sure why this username/password combo worked"); } catch (ContextAuthenticationException authException) { // pass } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } } // those were the first eight, now the ninth request // (with the same user and right pw) should fail - assertThrows(ContextAuthenticationException.class, () -> dao.authenticate("admin", "test")); + assertThrows(ContextAuthenticationException.class, () -> { + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + dao.authenticate("admin", "test"); + } finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } + }); } @Test diff --git a/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java b/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java index 86498b7212a5..e4600d0a0227 100644 --- a/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java +++ b/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java @@ -27,6 +27,7 @@ import org.openmrs.module.ModuleInteroperabilityTest; import org.openmrs.module.ModuleUtil; import org.openmrs.util.OpenmrsClassLoader; +import org.openmrs.util.PrivilegeConstants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.config.BeanDefinition; @@ -89,12 +90,16 @@ public void prepareTestInstance(TestContext testContext) throws Exception { Properties props = BaseContextSensitiveTest.runtimeProperties; props.setProperty(ModuleConstants.RUNTIMEPROPERTY_MODULE_LIST_TO_LOAD, modulesToLoad); try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); ModuleUtil.startup(props); } catch (Exception e) { log.error("Error while starting modules: ", e); throw e; } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } assertTrue("Some of the modules did not start successfully for " + testContext.getTestClass().getSimpleName() + ". Only " + ModuleFactory.getStartedModules().size() + " modules started instead of " + startModuleAnnotation.value().length, startModuleAnnotation @@ -160,6 +165,7 @@ public void afterTestClass(TestContext testContext) throws Exception { if (!Context.isSessionOpen()) { Context.openSession(); } +// Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); // re-registering the bean definitions that we may have removed for (String beanName : filteredDefinitions.keySet()) { diff --git a/api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java b/api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java index 7d15c3e53b0f..1a0dd45e2dd1 100644 --- a/api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java +++ b/api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java @@ -28,6 +28,7 @@ import org.openmrs.module.ModuleUtil; import org.openmrs.test.StartModule; import org.openmrs.util.OpenmrsClassLoader; +import org.openmrs.util.PrivilegeConstants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.config.BeanDefinition; @@ -78,6 +79,7 @@ public void prepareTestInstance(TestContext testContext) throws Exception { if (!Context.isSessionOpen()) Context.openSession(); + ModuleUtil.shutdown(); // load the omods that the dev defined for this class @@ -86,12 +88,16 @@ public void prepareTestInstance(TestContext testContext) throws Exception { Properties props = BaseContextSensitiveTest.runtimeProperties; props.setProperty(ModuleConstants.RUNTIMEPROPERTY_MODULE_LIST_TO_LOAD, modulesToLoad); try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); ModuleUtil.startup(props); } catch (Exception e) { log.error("Error while starting modules: ", e); throw e; } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } assertTrue("Some of the modules did not start successfully for " + testContext.getTestClass().getSimpleName() + ". Only " + ModuleFactory.getStartedModules().size() + " modules started instead of " + startModuleAnnotation.value().length, startModuleAnnotation diff --git a/api/src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml b/api/src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml index 71261c02d13f..3d248f93b442 100644 --- a/api/src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml +++ b/api/src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml @@ -21,8 +21,8 @@ - - + + diff --git a/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java b/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java index 1f3f22c4f6aa..a185b0ac353c 100644 --- a/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java +++ b/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java @@ -19,6 +19,7 @@ import org.openmrs.api.APIException; import org.openmrs.api.context.Context; import org.openmrs.util.OpenmrsConstants; +import org.openmrs.util.PrivilegeConstants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.web.filter.OncePerRequestFilter; @@ -124,9 +125,9 @@ private boolean isGZIPEnabled() { } try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); String gzipEnabled = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ENABLED, ""); - cachedGZipEnabledFlag = Boolean.valueOf(gzipEnabled); return cachedGZipEnabledFlag; } @@ -137,6 +138,9 @@ private boolean isGZIPEnabled() { return false; } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } } /** @@ -144,6 +148,7 @@ private boolean isGZIPEnabled() { */ private boolean isCompressedRequestForPathAccepted(String path) { try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); if (cachedGZipCompressedRequestForPathAccepted == null) { cachedGZipCompressedRequestForPathAccepted = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ACCEPT_COMPRESSED_REQUESTS_FOR_PATHS, ""); @@ -162,5 +167,8 @@ private boolean isCompressedRequestForPathAccepted(String path) { + OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ACCEPT_COMPRESSED_REQUESTS_FOR_PATHS, e); return false; } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } } } From 36add4c6ef4bf8cb5e09ea92e230f320bf43e72a Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Sat, 11 May 2024 18:14:52 +0530 Subject: [PATCH 02/20] TRUNK-6203: Global properties access should be privileged --- .../main/java/org/openmrs/web/filter/GZIPFilter.java | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java b/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java index a185b0ac353c..1f3f22c4f6aa 100644 --- a/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java +++ b/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java @@ -19,7 +19,6 @@ import org.openmrs.api.APIException; import org.openmrs.api.context.Context; import org.openmrs.util.OpenmrsConstants; -import org.openmrs.util.PrivilegeConstants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.web.filter.OncePerRequestFilter; @@ -125,9 +124,9 @@ private boolean isGZIPEnabled() { } try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); String gzipEnabled = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ENABLED, ""); + cachedGZipEnabledFlag = Boolean.valueOf(gzipEnabled); return cachedGZipEnabledFlag; } @@ -138,9 +137,6 @@ private boolean isGZIPEnabled() { return false; } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } } /** @@ -148,7 +144,6 @@ private boolean isGZIPEnabled() { */ private boolean isCompressedRequestForPathAccepted(String path) { try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); if (cachedGZipCompressedRequestForPathAccepted == null) { cachedGZipCompressedRequestForPathAccepted = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ACCEPT_COMPRESSED_REQUESTS_FOR_PATHS, ""); @@ -167,8 +162,5 @@ private boolean isCompressedRequestForPathAccepted(String path) { + OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ACCEPT_COMPRESSED_REQUESTS_FOR_PATHS, e); return false; } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } } } From 4cc16f2f734766eb63818f0df4b4877d88351bfc Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Sat, 11 May 2024 18:15:48 +0530 Subject: [PATCH 03/20] TRUNK-6203: Global properties access should be privileged --- api/src/main/java/org/openmrs/util/LocaleUtility.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/src/main/java/org/openmrs/util/LocaleUtility.java b/api/src/main/java/org/openmrs/util/LocaleUtility.java index 7fb00b43a6d7..547c1e0384d4 100644 --- a/api/src/main/java/org/openmrs/util/LocaleUtility.java +++ b/api/src/main/java/org/openmrs/util/LocaleUtility.java @@ -57,7 +57,6 @@ public static Locale getDefaultLocale() { if (defaultLocaleCache == null) { if (Context.isSessionOpen()) { try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); String locale = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_DEFAULT_LOCALE); @@ -75,9 +74,6 @@ public static Locale getDefaultLocale() { log.warn("Unable to get locale global property value. " + e.getMessage()); log.trace("Unable to get locale global property value", e); } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } // if we weren't able to load the locale from the global property, // use the default one From 1a8b78922788ea465aa5ef0b5a959e38dae79b1b Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Tue, 14 May 2024 10:06:06 +0530 Subject: [PATCH 04/20] TRUNK-6203: Global properties access should be privileged --- .../org/openmrs/api/db/ContextDAOTest.java | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java index 7e6525934253..a3729aab4c8f 100644 --- a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java +++ b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java @@ -306,20 +306,23 @@ public void authenticate_shouldPassRegressionTestFor1580() { } Context.logout(); - for (int x = 1; x <= 8; x++) { - // now we fail several login attempts - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - dao.authenticate("admin", "not the right password"); - fail("Not sure why this username/password combo worked"); - } - catch (ContextAuthenticationException authException) { - // pass - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + for (int x = 1; x <= 8; x++) { + // now we fail several login attempts + try { + dao.authenticate("admin", "not the right password"); + fail("Not sure why this username/password combo worked"); + } + catch (ContextAuthenticationException authException) { + // pass + } + } } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } + // those were the first eight, now the ninth request // (with the same user and right pw) should fail From b4a7b9ba309ee96345d9ddfe5da1b2aa91ffce4e Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Tue, 14 May 2024 20:10:09 +0530 Subject: [PATCH 05/20] TRUNK-6203: Global properties access should be privileged --- .../annotation/StartModuleAnnotationTest.java | 2 - .../api/AdministrationServiceTest.java | 21 ++++----- .../java/org/openmrs/api/UserServiceTest.java | 43 +++++------------ .../org/openmrs/api/db/ContextDAOTest.java | 46 ++++++------------- 4 files changed, 35 insertions(+), 77 deletions(-) diff --git a/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java b/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java index da5587f1b517..6c092ea2d2f4 100644 --- a/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java +++ b/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java @@ -10,11 +10,9 @@ package org.openmrs.annotation; import org.junit.jupiter.api.Test; -import org.openmrs.Role; import org.openmrs.api.context.Context; import org.openmrs.test.StartModule; import org.openmrs.test.jupiter.BaseContextSensitiveTest; -import org.openmrs.util.RoleConstants; import static org.junit.jupiter.api.Assertions.assertNotNull; diff --git a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java index 081d38ef218b..aa09e55af87d 100644 --- a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java +++ b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java @@ -20,7 +20,6 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.mock; import java.util.ArrayList; @@ -40,7 +39,6 @@ import org.openmrs.User; import org.openmrs.api.context.Context; import org.openmrs.api.context.Credentials; -import org.openmrs.api.context.UserContext; import org.openmrs.api.context.UsernamePasswordCredentials; import org.openmrs.customdatatype.datatype.BooleanDatatype; import org.openmrs.customdatatype.datatype.DateDatatype; @@ -564,19 +562,16 @@ public void filterGlobalPropertiesByViewPrivilege_shouldFilterGlobalPropertiesIf Context.logout(); Context.authenticate(getTestUserCredentials()); // have to add privilege in order to be able to call getAllGlobalProperties() method for new user + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - properties = adminService.getAllGlobalProperties(); - int actualSize = properties.size(); - Context.logout(); - assertEquals(actualSize, originalSize); - assertTrue(!properties.contains(property)); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + properties = adminService.getAllGlobalProperties(); + int actualSize = properties.size(); + + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + Context.logout(); + assertEquals(actualSize, originalSize); + assertTrue(!properties.contains(property)); } /** diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index 85fdfe06e8f1..6e4d01bbae83 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -496,15 +496,9 @@ public void changePassword_shouldMatchOnIncorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("incorrectlyhashedSha1", "test"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePassword("test", "Tester12"); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } - - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } @@ -544,14 +538,9 @@ public void changePassword_shouldMatchOnCorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("correctlyhashedSha1", "test"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePassword("test", "Tester12"); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } @@ -577,13 +566,9 @@ public void changePassword_shouldMatchOnSha512HashedPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("userWithSha512Hash", "test"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePassword("test", "Tester12"); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } @@ -1629,13 +1614,9 @@ public void changePasswordUsingActivationKey_shouldUpdatePasswordIfActivationKey final String PASSWORD = "Admin123"; Context.authenticate(createdUser.getUsername(), "Openmr5xy"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePasswordUsingActivationKey(key, PASSWORD); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePasswordUsingActivationKey(key, PASSWORD); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.authenticate(createdUser.getUsername(), PASSWORD); } diff --git a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java index a3729aab4c8f..540df393407e 100644 --- a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java +++ b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java @@ -279,61 +279,45 @@ public void authenticate_shouldPassRegressionTestFor1580() { // logout after the base setup Context.logout(); - // first we fail a login attempt + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "not the right password"); fail("Not sure why this username/password combo worked"); } catch (ContextAuthenticationException authException) { // pass } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } // next we log in correctly try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "test"); } catch (ContextAuthenticationException authException) { fail("There must be an admin:test user for this test to run properly"); } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } Context.logout(); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - for (int x = 1; x <= 8; x++) { - // now we fail several login attempts - try { - dao.authenticate("admin", "not the right password"); - fail("Not sure why this username/password combo worked"); - } - catch (ContextAuthenticationException authException) { - // pass - } - } - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + for (int x = 1; x <= 8; x++) { + // now we fail several login attempts + try { + dao.authenticate("admin", "not the right password"); + fail("Not sure why this username/password combo worked"); + } + catch (ContextAuthenticationException authException) { + // pass + } } - // those were the first eight, now the ninth request // (with the same user and right pw) should fail assertThrows(ContextAuthenticationException.class, () -> { - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - dao.authenticate("admin", "test"); - } finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + dao.authenticate("admin", "test"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); }); + + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); } @Test From d0f3b9be5d04fba4f670d4f3da043e4ad4678078 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 15 May 2024 15:07:12 +0530 Subject: [PATCH 06/20] TRUNK-6203: Global properties access should be privileged --- .../test/java/org/openmrs/api/UserServiceTest.java | 12 +++--------- .../openmrs/test/StartModuleExecutionListener.java | 1 - 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index 6e4d01bbae83..037b3e4e232b 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -1514,15 +1514,9 @@ public void changePasswordUsingSecretAnswer_shouldUpdatePasswordIfSecretIsCorrec User user = userService.getUser(6001); assertFalse(user.hasPrivilege(PrivilegeConstants.EDIT_USER_PASSWORDS)); Context.authenticate(user.getUsername(), "userServiceTest"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePasswordUsingSecretAnswer("answer", "userServiceTest2"); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } - - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePasswordUsingSecretAnswer("answer", "userServiceTest2"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.authenticate(user.getUsername(), "userServiceTest2"); } diff --git a/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java b/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java index e4600d0a0227..ce724b21e748 100644 --- a/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java +++ b/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java @@ -165,7 +165,6 @@ public void afterTestClass(TestContext testContext) throws Exception { if (!Context.isSessionOpen()) { Context.openSession(); } -// Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); // re-registering the bean definitions that we may have removed for (String beanName : filteredDefinitions.keySet()) { From 5a2f188131130450f364f59bff2255698791834a Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 15 May 2024 16:57:41 +0530 Subject: [PATCH 07/20] TRUNK-6203: Global properties access should be privileged --- api/src/test/java/org/openmrs/api/UserServiceTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index 037b3e4e232b..b255f4453322 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -568,7 +568,6 @@ public void changePassword_shouldMatchOnSha512HashedPassword() { Context.authenticate("userWithSha512Hash", "test"); Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } From 574c6da4c6093c5f5aa2b648714f4bfbfeca56a9 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 15 May 2024 17:07:43 +0530 Subject: [PATCH 08/20] TRUNK-6203: Global properties access should be privileged --- .../test/java/org/openmrs/api/AdministrationServiceTest.java | 4 +++- api/src/test/java/org/openmrs/api/UserServiceTest.java | 2 -- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java index aa09e55af87d..9b6c2753b19e 100644 --- a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java +++ b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java @@ -669,7 +669,6 @@ public void updateGlobalProperty_shouldFailIfUserIsNotAllowedToEditGlobalPropert public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty() { executeDataSet(ADMIN_INITIAL_DATA_XML); GlobalProperty property = getGlobalPropertyWithEditPrivilege(); - GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); assertEquals("anothervalue", property.getPropertyValue()); // authenticate new user without privileges @@ -678,7 +677,10 @@ public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty // add required privilege to user Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getEditPrivilege()); + + GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); role.addPrivilege(globalPropertyWithViewPrivilege.getViewPrivilege()); + Context.getAuthenticatedUser().addRole(role); adminService.updateGlobalProperty(property.getProperty(), "new-value"); diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index b255f4453322..c59322f7b276 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -498,7 +498,6 @@ public void changePassword_shouldMatchOnIncorrectlyHashedSha1StoredPassword() { Context.authenticate("incorrectlyhashedSha1", "test"); Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } @@ -540,7 +539,6 @@ public void changePassword_shouldMatchOnCorrectlyHashedSha1StoredPassword() { Context.authenticate("correctlyhashedSha1", "test"); Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } From 566eb789170f01f06748b9488f36370c738856d8 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 15 May 2024 17:25:35 +0530 Subject: [PATCH 09/20] TRUNK-6203: Global properties access should be privileged --- .../test/java/org/openmrs/api/AdministrationServiceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java index 9b6c2753b19e..9227815970ea 100644 --- a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java +++ b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java @@ -669,6 +669,7 @@ public void updateGlobalProperty_shouldFailIfUserIsNotAllowedToEditGlobalPropert public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty() { executeDataSet(ADMIN_INITIAL_DATA_XML); GlobalProperty property = getGlobalPropertyWithEditPrivilege(); + GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); assertEquals("anothervalue", property.getPropertyValue()); // authenticate new user without privileges @@ -678,7 +679,6 @@ public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getEditPrivilege()); - GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); role.addPrivilege(globalPropertyWithViewPrivilege.getViewPrivilege()); Context.getAuthenticatedUser().addRole(role); From 0085916a18ae4568ef9b88b2adb3c1c06e55bc67 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 20 Mar 2024 17:45:11 +0530 Subject: [PATCH 10/20] TRUNK-6203: Global properties access should be privileged --- .../openmrs/api/AdministrationService.java | 9 ++++ .../java/org/openmrs/util/LocaleUtility.java | 4 ++ .../annotation/StartModuleAnnotationTest.java | 8 +-- .../openmrs/aop/AuthorizationAdviceTest.java | 2 +- .../api/AdministrationServiceTest.java | 41 +++++++++------- .../java/org/openmrs/api/UserServiceTest.java | 49 +++++++++++++++---- .../org/openmrs/api/db/ContextDAOTest.java | 23 ++++++++- .../test/StartModuleExecutionListener.java | 6 +++ .../jupiter/StartModuleExecutionListener.java | 6 +++ ...nistrationServiceTest-globalproperties.xml | 4 +- .../org/openmrs/web/filter/GZIPFilter.java | 10 +++- 11 files changed, 125 insertions(+), 37 deletions(-) diff --git a/api/src/main/java/org/openmrs/api/AdministrationService.java b/api/src/main/java/org/openmrs/api/AdministrationService.java index de1466f38e8d..f25d2c7c1e9c 100644 --- a/api/src/main/java/org/openmrs/api/AdministrationService.java +++ b/api/src/main/java/org/openmrs/api/AdministrationService.java @@ -56,6 +56,7 @@ public interface AdministrationService extends OpenmrsService { * Should find object given valid uuid * Should return null if no object found with given uuid */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public GlobalProperty getGlobalPropertyByUuid(String uuid); /** @@ -90,6 +91,7 @@ public interface AdministrationService extends OpenmrsService { * Should get property value given valid property name * Should get property in case insensitive way */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public String getGlobalProperty(String propertyName); /** @@ -106,6 +108,7 @@ public interface AdministrationService extends OpenmrsService { * Should return default value if property name does not exist * Should not fail with null default value */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public String getGlobalProperty(String propertyName, String defaultValue); /** @@ -115,6 +118,7 @@ public interface AdministrationService extends OpenmrsService { * @return the global property that matches the given propertyName * Should return null when no global property match given property name */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public GlobalProperty getGlobalPropertyObject(String propertyName); /** @@ -125,6 +129,7 @@ public interface AdministrationService extends OpenmrsService { * @since 1.5 * Should return all relevant global properties in the database */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public List getGlobalPropertiesByPrefix(String prefix); /** @@ -135,6 +140,7 @@ public interface AdministrationService extends OpenmrsService { * @since 1.6 * Should return all relevant global properties in the database */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public List getGlobalPropertiesBySuffix(String suffix); /** @@ -189,6 +195,7 @@ public interface AdministrationService extends OpenmrsService { * Should overwrite global property if exists * Should save a global property whose typed value is handled by a custom datatype */ + @Authorized(PrivilegeConstants.MANAGE_GLOBAL_PROPERTIES) public void setGlobalProperty(String propertyName, String propertyValue); /** @@ -202,6 +209,7 @@ public interface AdministrationService extends OpenmrsService { * Should fail if global property being updated does not already exist * Should update a global property whose typed value is handled by a custom datatype */ + @Authorized(PrivilegeConstants.MANAGE_GLOBAL_PROPERTIES) public void updateGlobalProperty(String propertyName, String propertyValue); /** @@ -313,6 +321,7 @@ public interface AdministrationService extends OpenmrsService { * @return property value in the type of the default value * @since 1.7 */ + @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) public T getGlobalPropertyValue(String propertyName, T defaultValue); /** diff --git a/api/src/main/java/org/openmrs/util/LocaleUtility.java b/api/src/main/java/org/openmrs/util/LocaleUtility.java index 547c1e0384d4..7fb00b43a6d7 100644 --- a/api/src/main/java/org/openmrs/util/LocaleUtility.java +++ b/api/src/main/java/org/openmrs/util/LocaleUtility.java @@ -57,6 +57,7 @@ public static Locale getDefaultLocale() { if (defaultLocaleCache == null) { if (Context.isSessionOpen()) { try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); String locale = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_DEFAULT_LOCALE); @@ -74,6 +75,9 @@ public static Locale getDefaultLocale() { log.warn("Unable to get locale global property value. " + e.getMessage()); log.trace("Unable to get locale global property value", e); } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } // if we weren't able to load the locale from the global property, // use the default one diff --git a/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java b/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java index 573149df7ebc..da5587f1b517 100644 --- a/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java +++ b/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java @@ -9,12 +9,14 @@ */ package org.openmrs.annotation; -import static org.junit.jupiter.api.Assertions.assertNotNull; - import org.junit.jupiter.api.Test; +import org.openmrs.Role; import org.openmrs.api.context.Context; -import org.openmrs.test.jupiter.BaseContextSensitiveTest; import org.openmrs.test.StartModule; +import org.openmrs.test.jupiter.BaseContextSensitiveTest; +import org.openmrs.util.RoleConstants; + +import static org.junit.jupiter.api.Assertions.assertNotNull; @StartModule({ "org/openmrs/module/include/test1-1.0-SNAPSHOT.omod", "org/openmrs/module/include/test2-1.0-SNAPSHOT.omod" }) public class StartModuleAnnotationTest extends BaseContextSensitiveTest { diff --git a/api/src/test/java/org/openmrs/aop/AuthorizationAdviceTest.java b/api/src/test/java/org/openmrs/aop/AuthorizationAdviceTest.java index 851de73e36bf..b60798c05503 100644 --- a/api/src/test/java/org/openmrs/aop/AuthorizationAdviceTest.java +++ b/api/src/test/java/org/openmrs/aop/AuthorizationAdviceTest.java @@ -65,7 +65,7 @@ public void before_shouldNotifyListenersAboutCheckedPrivileges() { Context.getConceptService().saveConcept(concept); String[] privileges = { PrivilegeConstants.MANAGE_CONCEPTS, PrivilegeConstants.GET_OBS, - PrivilegeConstants.GET_CONCEPT_ATTRIBUTE_TYPES }; + PrivilegeConstants.GET_CONCEPT_ATTRIBUTE_TYPES, PrivilegeConstants.GET_GLOBAL_PROPERTIES }; assertThat("listener1", listener1.hasPrivileges, containsInAnyOrder(privileges)); assertThat("listener2", listener2.hasPrivileges, containsInAnyOrder(privileges)); assertThat(listener1.lacksPrivileges, empty()); diff --git a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java index 27c59f3f63ac..081d38ef218b 100644 --- a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java +++ b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java @@ -554,7 +554,7 @@ public void filterGlobalPropertiesByViewPrivilege_shouldFilterGlobalPropertiesIf GlobalProperty property = new GlobalProperty(); property.setProperty("test_property"); property.setPropertyValue("test_property_value"); - property.setViewPrivilege(Context.getUserService().getPrivilege("Some Privilege For View Global Properties")); + property.setViewPrivilege(Context.getUserService().getPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES)); adminService.saveGlobalProperty(property); // assert new test global property is saved properly List properties = adminService.getAllGlobalProperties(); @@ -564,16 +564,19 @@ public void filterGlobalPropertiesByViewPrivilege_shouldFilterGlobalPropertiesIf Context.logout(); Context.authenticate(getTestUserCredentials()); // have to add privilege in order to be able to call getAllGlobalProperties() method for new user - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - properties = adminService.getAllGlobalProperties(); - int actualSize = properties.size(); - - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - Context.logout(); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + properties = adminService.getAllGlobalProperties(); + int actualSize = properties.size(); + Context.logout(); + assertEquals(actualSize, originalSize); + assertTrue(!properties.contains(property)); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } - assertEquals(actualSize, originalSize); - assertTrue(!properties.contains(property)); } /** @@ -588,9 +591,8 @@ public void getGlobalProperty_shouldFailIfUserHasNoPrivileges() { Context.logout(); Context.authenticate(getTestUserCredentials()); - APIException exception = assertThrows(APIException.class, () -> adminService.getGlobalProperty(property.getProperty())); - assertEquals(exception.getMessage(), String.format("Privilege: %s, required to view globalProperty: %s", - property.getViewPrivilege(), property.getProperty())); + APIAuthenticationException exception = assertThrows(APIAuthenticationException.class, () -> adminService.getGlobalProperty(property.getProperty())); + assertEquals(exception.getMessage(), String.format("Privileges required: %s", property.getViewPrivilege())); } /** @@ -608,7 +610,6 @@ public void getGlobalProperty_shouldReturnGlobalPropertyIfUserIsAllowedToView() Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getViewPrivilege()); Context.getAuthenticatedUser().addRole(role); - assertNotNull(adminService.getGlobalProperty(property.getProperty())); } @@ -625,8 +626,8 @@ public void getGlobalPropertyObject_shouldFailIfUserHasNoPrivileges() { Context.authenticate(getTestUserCredentials()); APIException exception = assertThrows(APIException.class, () -> adminService.getGlobalPropertyObject(property.getProperty())); - assertEquals(exception.getMessage(), String.format("Privilege: %s, required to view globalProperty: %s", - property.getViewPrivilege(), property.getProperty())); + assertEquals(exception.getMessage(), String.format("Privileges required: %s", + property.getViewPrivilege())); } /** @@ -662,8 +663,8 @@ public void updateGlobalProperty_shouldFailIfUserIsNotAllowedToEditGlobalPropert Context.authenticate(getTestUserCredentials()); APIException exception = assertThrows(APIException.class, () -> adminService.updateGlobalProperty(property.getProperty(), "new-value")); - assertEquals(exception.getMessage(), String.format("Privilege: %s, required to edit globalProperty: %s", - property.getEditPrivilege(), property.getProperty())); + assertEquals(exception.getMessage(), String.format("Privileges required: %s", + property.getEditPrivilege())); } /** @@ -673,6 +674,7 @@ public void updateGlobalProperty_shouldFailIfUserIsNotAllowedToEditGlobalPropert public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty() { executeDataSet(ADMIN_INITIAL_DATA_XML); GlobalProperty property = getGlobalPropertyWithEditPrivilege(); + GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); assertEquals("anothervalue", property.getPropertyValue()); // authenticate new user without privileges @@ -681,6 +683,7 @@ public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty // add required privilege to user Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getEditPrivilege()); + role.addPrivilege(globalPropertyWithViewPrivilege.getViewPrivilege()); Context.getAuthenticatedUser().addRole(role); adminService.updateGlobalProperty(property.getProperty(), "new-value"); @@ -735,7 +738,7 @@ private GlobalProperty getGlobalPropertyWithViewPrivilege() { GlobalProperty property = adminService.getGlobalPropertyObject("another-global-property"); assertNotNull(property); - Privilege viewPrivilege = Context.getUserService().getPrivilege("Some Privilege For View Global Properties"); + Privilege viewPrivilege = Context.getUserService().getPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); property.setViewPrivilege(viewPrivilege); property = adminService.saveGlobalProperty(property); assertNotNull(property.getViewPrivilege()); @@ -752,7 +755,7 @@ private GlobalProperty getGlobalPropertyWithEditPrivilege() { GlobalProperty property = adminService.getGlobalPropertyObject("another-global-property"); assertNotNull(property); - Privilege editPrivilege = Context.getUserService().getPrivilege("Some Privilege For Edit Global Properties"); + Privilege editPrivilege = Context.getUserService().getPrivilege(PrivilegeConstants.MANAGE_GLOBAL_PROPERTIES); property.setEditPrivilege(editPrivilege); property = adminService.saveGlobalProperty(property); assertNotNull(property.getEditPrivilege()); diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index af2f6b6324dd..85fdfe06e8f1 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -260,6 +260,7 @@ public void createUser_shouldNotAllowCreatingUserWithPrivilegeCurrentUserDoesNot Role userRole = new Role("User Adder"); userRole.setRole(RoleConstants.AUTHENTICATED); userRole.addPrivilege(new Privilege("Add Users")); + userRole.addPrivilege(new Privilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES)); currentUser.addRole(userRole); // setup our expected exception // we expect this to fail because the currently logged-in user lacks a privilege to be @@ -293,6 +294,7 @@ public void createUser_shouldNotAllowCreatingUserWithPrivilegesCurrentUserDoesNo Role userRole = new Role("User Adder"); userRole.setRole(RoleConstants.AUTHENTICATED); userRole.addPrivilege(new Privilege("Add Users")); + userRole.addPrivilege(new Privilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES)); currentUser.addRole(userRole); // setup our expected exception // we expect this to fail because the currently logged-in user lacks a privilege to be @@ -327,8 +329,8 @@ public void createUser_shouldNotAllowAssigningSuperUserRoleIfCurrentUserDoesNotH Role userRole = new Role("User Adder"); userRole.setRole(RoleConstants.AUTHENTICATED); userRole.addPrivilege(new Privilege("Add Users")); + userRole.addPrivilege(new Privilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES)); currentUser.addRole(userRole); - // setup our expected exception // we expect this to fail because the currently logged-in user lacks a privilege to be // assigned to the new user @@ -494,8 +496,14 @@ public void changePassword_shouldMatchOnIncorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("incorrectlyhashedSha1", "test"); - - userService.changePassword("test", "Tester12"); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } + Context.logout(); // so that the next test reauthenticates } @@ -536,8 +544,13 @@ public void changePassword_shouldMatchOnCorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("correctlyhashedSha1", "test"); - - userService.changePassword("test", "Tester12"); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } Context.logout(); // so that the next test reauthenticates } @@ -564,9 +577,13 @@ public void changePassword_shouldMatchOnSha512HashedPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("userWithSha512Hash", "test"); - - userService.changePassword("test", "Tester12"); - + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } Context.logout(); // so that the next test reauthenticates } @@ -1512,8 +1529,14 @@ public void changePasswordUsingSecretAnswer_shouldUpdatePasswordIfSecretIsCorrec User user = userService.getUser(6001); assertFalse(user.hasPrivilege(PrivilegeConstants.EDIT_USER_PASSWORDS)); Context.authenticate(user.getUsername(), "userServiceTest"); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePasswordUsingSecretAnswer("answer", "userServiceTest2"); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } - userService.changePasswordUsingSecretAnswer("answer", "userServiceTest2"); Context.authenticate(user.getUsername(), "userServiceTest2"); } @@ -1606,7 +1629,13 @@ public void changePasswordUsingActivationKey_shouldUpdatePasswordIfActivationKey final String PASSWORD = "Admin123"; Context.authenticate(createdUser.getUsername(), "Openmr5xy"); - userService.changePasswordUsingActivationKey(key, PASSWORD); + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePasswordUsingActivationKey(key, PASSWORD); + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } Context.authenticate(createdUser.getUsername(), PASSWORD); } diff --git a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java index 688c5cfbf3f3..7e6525934253 100644 --- a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java +++ b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java @@ -32,6 +32,7 @@ import org.openmrs.api.context.ContextAuthenticationException; import org.openmrs.api.db.hibernate.HibernateContextDAO; import org.openmrs.test.jupiter.BaseContextSensitiveTest; +import org.openmrs.util.PrivilegeConstants; import org.springframework.stereotype.Component; /** @@ -278,38 +279,58 @@ public void authenticate_shouldPassRegressionTestFor1580() { // logout after the base setup Context.logout(); + // first we fail a login attempt try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "not the right password"); fail("Not sure why this username/password combo worked"); } catch (ContextAuthenticationException authException) { // pass } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } // next we log in correctly try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "test"); } catch (ContextAuthenticationException authException) { fail("There must be an admin:test user for this test to run properly"); } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } Context.logout(); for (int x = 1; x <= 8; x++) { // now we fail several login attempts try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "not the right password"); fail("Not sure why this username/password combo worked"); } catch (ContextAuthenticationException authException) { // pass } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } } // those were the first eight, now the ninth request // (with the same user and right pw) should fail - assertThrows(ContextAuthenticationException.class, () -> dao.authenticate("admin", "test")); + assertThrows(ContextAuthenticationException.class, () -> { + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + dao.authenticate("admin", "test"); + } finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } + }); } @Test diff --git a/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java b/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java index 86498b7212a5..e4600d0a0227 100644 --- a/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java +++ b/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java @@ -27,6 +27,7 @@ import org.openmrs.module.ModuleInteroperabilityTest; import org.openmrs.module.ModuleUtil; import org.openmrs.util.OpenmrsClassLoader; +import org.openmrs.util.PrivilegeConstants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.config.BeanDefinition; @@ -89,12 +90,16 @@ public void prepareTestInstance(TestContext testContext) throws Exception { Properties props = BaseContextSensitiveTest.runtimeProperties; props.setProperty(ModuleConstants.RUNTIMEPROPERTY_MODULE_LIST_TO_LOAD, modulesToLoad); try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); ModuleUtil.startup(props); } catch (Exception e) { log.error("Error while starting modules: ", e); throw e; } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } assertTrue("Some of the modules did not start successfully for " + testContext.getTestClass().getSimpleName() + ". Only " + ModuleFactory.getStartedModules().size() + " modules started instead of " + startModuleAnnotation.value().length, startModuleAnnotation @@ -160,6 +165,7 @@ public void afterTestClass(TestContext testContext) throws Exception { if (!Context.isSessionOpen()) { Context.openSession(); } +// Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); // re-registering the bean definitions that we may have removed for (String beanName : filteredDefinitions.keySet()) { diff --git a/api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java b/api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java index 7d15c3e53b0f..1a0dd45e2dd1 100644 --- a/api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java +++ b/api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java @@ -28,6 +28,7 @@ import org.openmrs.module.ModuleUtil; import org.openmrs.test.StartModule; import org.openmrs.util.OpenmrsClassLoader; +import org.openmrs.util.PrivilegeConstants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.config.BeanDefinition; @@ -78,6 +79,7 @@ public void prepareTestInstance(TestContext testContext) throws Exception { if (!Context.isSessionOpen()) Context.openSession(); + ModuleUtil.shutdown(); // load the omods that the dev defined for this class @@ -86,12 +88,16 @@ public void prepareTestInstance(TestContext testContext) throws Exception { Properties props = BaseContextSensitiveTest.runtimeProperties; props.setProperty(ModuleConstants.RUNTIMEPROPERTY_MODULE_LIST_TO_LOAD, modulesToLoad); try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); ModuleUtil.startup(props); } catch (Exception e) { log.error("Error while starting modules: ", e); throw e; } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } assertTrue("Some of the modules did not start successfully for " + testContext.getTestClass().getSimpleName() + ". Only " + ModuleFactory.getStartedModules().size() + " modules started instead of " + startModuleAnnotation.value().length, startModuleAnnotation diff --git a/api/src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml b/api/src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml index 71261c02d13f..3d248f93b442 100644 --- a/api/src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml +++ b/api/src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml @@ -21,8 +21,8 @@ - - + + diff --git a/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java b/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java index 1f3f22c4f6aa..a185b0ac353c 100644 --- a/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java +++ b/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java @@ -19,6 +19,7 @@ import org.openmrs.api.APIException; import org.openmrs.api.context.Context; import org.openmrs.util.OpenmrsConstants; +import org.openmrs.util.PrivilegeConstants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.web.filter.OncePerRequestFilter; @@ -124,9 +125,9 @@ private boolean isGZIPEnabled() { } try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); String gzipEnabled = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ENABLED, ""); - cachedGZipEnabledFlag = Boolean.valueOf(gzipEnabled); return cachedGZipEnabledFlag; } @@ -137,6 +138,9 @@ private boolean isGZIPEnabled() { return false; } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } } /** @@ -144,6 +148,7 @@ private boolean isGZIPEnabled() { */ private boolean isCompressedRequestForPathAccepted(String path) { try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); if (cachedGZipCompressedRequestForPathAccepted == null) { cachedGZipCompressedRequestForPathAccepted = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ACCEPT_COMPRESSED_REQUESTS_FOR_PATHS, ""); @@ -162,5 +167,8 @@ private boolean isCompressedRequestForPathAccepted(String path) { + OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ACCEPT_COMPRESSED_REQUESTS_FOR_PATHS, e); return false; } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } } } From 959ccfb7b7653399fef16893bba6a8b50ad902e5 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Sat, 11 May 2024 18:14:52 +0530 Subject: [PATCH 11/20] TRUNK-6203: Global properties access should be privileged --- .../main/java/org/openmrs/web/filter/GZIPFilter.java | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java b/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java index a185b0ac353c..1f3f22c4f6aa 100644 --- a/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java +++ b/web/src/main/java/org/openmrs/web/filter/GZIPFilter.java @@ -19,7 +19,6 @@ import org.openmrs.api.APIException; import org.openmrs.api.context.Context; import org.openmrs.util.OpenmrsConstants; -import org.openmrs.util.PrivilegeConstants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.web.filter.OncePerRequestFilter; @@ -125,9 +124,9 @@ private boolean isGZIPEnabled() { } try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); String gzipEnabled = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ENABLED, ""); + cachedGZipEnabledFlag = Boolean.valueOf(gzipEnabled); return cachedGZipEnabledFlag; } @@ -138,9 +137,6 @@ private boolean isGZIPEnabled() { return false; } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } } /** @@ -148,7 +144,6 @@ private boolean isGZIPEnabled() { */ private boolean isCompressedRequestForPathAccepted(String path) { try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); if (cachedGZipCompressedRequestForPathAccepted == null) { cachedGZipCompressedRequestForPathAccepted = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ACCEPT_COMPRESSED_REQUESTS_FOR_PATHS, ""); @@ -167,8 +162,5 @@ private boolean isCompressedRequestForPathAccepted(String path) { + OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ACCEPT_COMPRESSED_REQUESTS_FOR_PATHS, e); return false; } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } } } From 84403668a970f69ebb88f0d77c8080d9f120d5a8 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Sat, 11 May 2024 18:15:48 +0530 Subject: [PATCH 12/20] TRUNK-6203: Global properties access should be privileged --- api/src/main/java/org/openmrs/util/LocaleUtility.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/src/main/java/org/openmrs/util/LocaleUtility.java b/api/src/main/java/org/openmrs/util/LocaleUtility.java index 7fb00b43a6d7..547c1e0384d4 100644 --- a/api/src/main/java/org/openmrs/util/LocaleUtility.java +++ b/api/src/main/java/org/openmrs/util/LocaleUtility.java @@ -57,7 +57,6 @@ public static Locale getDefaultLocale() { if (defaultLocaleCache == null) { if (Context.isSessionOpen()) { try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); String locale = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_DEFAULT_LOCALE); @@ -75,9 +74,6 @@ public static Locale getDefaultLocale() { log.warn("Unable to get locale global property value. " + e.getMessage()); log.trace("Unable to get locale global property value", e); } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } // if we weren't able to load the locale from the global property, // use the default one From 8bd9bd9bd61fc2f25fe1dbcd956ed208f09990f3 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Tue, 14 May 2024 10:06:06 +0530 Subject: [PATCH 13/20] TRUNK-6203: Global properties access should be privileged --- .../org/openmrs/api/db/ContextDAOTest.java | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java index 7e6525934253..a3729aab4c8f 100644 --- a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java +++ b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java @@ -306,20 +306,23 @@ public void authenticate_shouldPassRegressionTestFor1580() { } Context.logout(); - for (int x = 1; x <= 8; x++) { - // now we fail several login attempts - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - dao.authenticate("admin", "not the right password"); - fail("Not sure why this username/password combo worked"); - } - catch (ContextAuthenticationException authException) { - // pass - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + for (int x = 1; x <= 8; x++) { + // now we fail several login attempts + try { + dao.authenticate("admin", "not the right password"); + fail("Not sure why this username/password combo worked"); + } + catch (ContextAuthenticationException authException) { + // pass + } + } } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + } + // those were the first eight, now the ninth request // (with the same user and right pw) should fail From be1b829d682a8187cf3469360265dc2d8bdc1b65 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Tue, 14 May 2024 20:10:09 +0530 Subject: [PATCH 14/20] TRUNK-6203: Global properties access should be privileged --- .../annotation/StartModuleAnnotationTest.java | 2 - .../api/AdministrationServiceTest.java | 21 ++++----- .../java/org/openmrs/api/UserServiceTest.java | 43 +++++------------ .../org/openmrs/api/db/ContextDAOTest.java | 46 ++++++------------- 4 files changed, 35 insertions(+), 77 deletions(-) diff --git a/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java b/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java index da5587f1b517..6c092ea2d2f4 100644 --- a/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java +++ b/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java @@ -10,11 +10,9 @@ package org.openmrs.annotation; import org.junit.jupiter.api.Test; -import org.openmrs.Role; import org.openmrs.api.context.Context; import org.openmrs.test.StartModule; import org.openmrs.test.jupiter.BaseContextSensitiveTest; -import org.openmrs.util.RoleConstants; import static org.junit.jupiter.api.Assertions.assertNotNull; diff --git a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java index 081d38ef218b..aa09e55af87d 100644 --- a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java +++ b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java @@ -20,7 +20,6 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.mock; import java.util.ArrayList; @@ -40,7 +39,6 @@ import org.openmrs.User; import org.openmrs.api.context.Context; import org.openmrs.api.context.Credentials; -import org.openmrs.api.context.UserContext; import org.openmrs.api.context.UsernamePasswordCredentials; import org.openmrs.customdatatype.datatype.BooleanDatatype; import org.openmrs.customdatatype.datatype.DateDatatype; @@ -564,19 +562,16 @@ public void filterGlobalPropertiesByViewPrivilege_shouldFilterGlobalPropertiesIf Context.logout(); Context.authenticate(getTestUserCredentials()); // have to add privilege in order to be able to call getAllGlobalProperties() method for new user + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - properties = adminService.getAllGlobalProperties(); - int actualSize = properties.size(); - Context.logout(); - assertEquals(actualSize, originalSize); - assertTrue(!properties.contains(property)); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + properties = adminService.getAllGlobalProperties(); + int actualSize = properties.size(); + + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + Context.logout(); + assertEquals(actualSize, originalSize); + assertTrue(!properties.contains(property)); } /** diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index 85fdfe06e8f1..6e4d01bbae83 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -496,15 +496,9 @@ public void changePassword_shouldMatchOnIncorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("incorrectlyhashedSha1", "test"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePassword("test", "Tester12"); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } - - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } @@ -544,14 +538,9 @@ public void changePassword_shouldMatchOnCorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("correctlyhashedSha1", "test"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePassword("test", "Tester12"); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } @@ -577,13 +566,9 @@ public void changePassword_shouldMatchOnSha512HashedPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("userWithSha512Hash", "test"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePassword("test", "Tester12"); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePassword("test", "Tester12"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } @@ -1629,13 +1614,9 @@ public void changePasswordUsingActivationKey_shouldUpdatePasswordIfActivationKey final String PASSWORD = "Admin123"; Context.authenticate(createdUser.getUsername(), "Openmr5xy"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePasswordUsingActivationKey(key, PASSWORD); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePasswordUsingActivationKey(key, PASSWORD); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.authenticate(createdUser.getUsername(), PASSWORD); } diff --git a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java index a3729aab4c8f..540df393407e 100644 --- a/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java +++ b/api/src/test/java/org/openmrs/api/db/ContextDAOTest.java @@ -279,61 +279,45 @@ public void authenticate_shouldPassRegressionTestFor1580() { // logout after the base setup Context.logout(); - // first we fail a login attempt + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "not the right password"); fail("Not sure why this username/password combo worked"); } catch (ContextAuthenticationException authException) { // pass } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } // next we log in correctly try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); dao.authenticate("admin", "test"); } catch (ContextAuthenticationException authException) { fail("There must be an admin:test user for this test to run properly"); } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } Context.logout(); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - for (int x = 1; x <= 8; x++) { - // now we fail several login attempts - try { - dao.authenticate("admin", "not the right password"); - fail("Not sure why this username/password combo worked"); - } - catch (ContextAuthenticationException authException) { - // pass - } - } - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + for (int x = 1; x <= 8; x++) { + // now we fail several login attempts + try { + dao.authenticate("admin", "not the right password"); + fail("Not sure why this username/password combo worked"); + } + catch (ContextAuthenticationException authException) { + // pass + } } - // those were the first eight, now the ninth request // (with the same user and right pw) should fail assertThrows(ContextAuthenticationException.class, () -> { - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - dao.authenticate("admin", "test"); - } finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + dao.authenticate("admin", "test"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); }); + + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); } @Test From 081220d63bc9d9666d36b6031c299fa7ebdba5eb Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 15 May 2024 15:07:12 +0530 Subject: [PATCH 15/20] TRUNK-6203: Global properties access should be privileged --- .../test/java/org/openmrs/api/UserServiceTest.java | 12 +++--------- .../openmrs/test/StartModuleExecutionListener.java | 1 - 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index 6e4d01bbae83..037b3e4e232b 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -1514,15 +1514,9 @@ public void changePasswordUsingSecretAnswer_shouldUpdatePasswordIfSecretIsCorrec User user = userService.getUser(6001); assertFalse(user.hasPrivilege(PrivilegeConstants.EDIT_USER_PASSWORDS)); Context.authenticate(user.getUsername(), "userServiceTest"); - try { - Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - userService.changePasswordUsingSecretAnswer("answer", "userServiceTest2"); - } - finally { - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); - } - - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + userService.changePasswordUsingSecretAnswer("answer", "userServiceTest2"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.authenticate(user.getUsername(), "userServiceTest2"); } diff --git a/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java b/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java index e4600d0a0227..ce724b21e748 100644 --- a/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java +++ b/api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java @@ -165,7 +165,6 @@ public void afterTestClass(TestContext testContext) throws Exception { if (!Context.isSessionOpen()) { Context.openSession(); } -// Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); // re-registering the bean definitions that we may have removed for (String beanName : filteredDefinitions.keySet()) { From de74e917190ef947cc5affdb01ea8cf90f239e78 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 15 May 2024 16:57:41 +0530 Subject: [PATCH 16/20] TRUNK-6203: Global properties access should be privileged --- api/src/test/java/org/openmrs/api/UserServiceTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index 037b3e4e232b..b255f4453322 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -568,7 +568,6 @@ public void changePassword_shouldMatchOnSha512HashedPassword() { Context.authenticate("userWithSha512Hash", "test"); Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } From 3c9d1e4ccacf43c586fa3b85bfa83a9298be24a7 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 15 May 2024 17:07:43 +0530 Subject: [PATCH 17/20] TRUNK-6203: Global properties access should be privileged --- .../test/java/org/openmrs/api/AdministrationServiceTest.java | 4 +++- api/src/test/java/org/openmrs/api/UserServiceTest.java | 2 -- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java index aa09e55af87d..9b6c2753b19e 100644 --- a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java +++ b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java @@ -669,7 +669,6 @@ public void updateGlobalProperty_shouldFailIfUserIsNotAllowedToEditGlobalPropert public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty() { executeDataSet(ADMIN_INITIAL_DATA_XML); GlobalProperty property = getGlobalPropertyWithEditPrivilege(); - GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); assertEquals("anothervalue", property.getPropertyValue()); // authenticate new user without privileges @@ -678,7 +677,10 @@ public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty // add required privilege to user Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getEditPrivilege()); + + GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); role.addPrivilege(globalPropertyWithViewPrivilege.getViewPrivilege()); + Context.getAuthenticatedUser().addRole(role); adminService.updateGlobalProperty(property.getProperty(), "new-value"); diff --git a/api/src/test/java/org/openmrs/api/UserServiceTest.java b/api/src/test/java/org/openmrs/api/UserServiceTest.java index b255f4453322..c59322f7b276 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -498,7 +498,6 @@ public void changePassword_shouldMatchOnIncorrectlyHashedSha1StoredPassword() { Context.authenticate("incorrectlyhashedSha1", "test"); Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } @@ -540,7 +539,6 @@ public void changePassword_shouldMatchOnCorrectlyHashedSha1StoredPassword() { Context.authenticate("correctlyhashedSha1", "test"); Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); - Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.logout(); // so that the next test reauthenticates } From 7d4fe9400f2e94188aff2449a753c9ac07e285d7 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 15 May 2024 17:25:35 +0530 Subject: [PATCH 18/20] TRUNK-6203: Global properties access should be privileged --- .../test/java/org/openmrs/api/AdministrationServiceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java index 9b6c2753b19e..9227815970ea 100644 --- a/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java +++ b/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java @@ -669,6 +669,7 @@ public void updateGlobalProperty_shouldFailIfUserIsNotAllowedToEditGlobalPropert public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty() { executeDataSet(ADMIN_INITIAL_DATA_XML); GlobalProperty property = getGlobalPropertyWithEditPrivilege(); + GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); assertEquals("anothervalue", property.getPropertyValue()); // authenticate new user without privileges @@ -678,7 +679,6 @@ public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getEditPrivilege()); - GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege(); role.addPrivilege(globalPropertyWithViewPrivilege.getViewPrivilege()); Context.getAuthenticatedUser().addRole(role); From c0912d93c11245567d6f4d06437a8389a8a09270 Mon Sep 17 00:00:00 2001 From: Wikum Weerakutti Date: Thu, 20 Jun 2024 17:29:00 +0530 Subject: [PATCH 19/20] Removing random changes --- api/src/main/java/org/openmrs/Concept.java | 4 ++-- pom.xml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/org/openmrs/Concept.java b/api/src/main/java/org/openmrs/Concept.java index 8a63b0e1b4b0..ecd9833cd7eb 100644 --- a/api/src/main/java/org/openmrs/Concept.java +++ b/api/src/main/java/org/openmrs/Concept.java @@ -1436,9 +1436,9 @@ public List getPossibleValues() { * @see org.openmrs.Attributable#hydrate(java.lang.String) */ @Override - public Concept hydrate(String reference) { + public Concept hydrate(String s) { try { - return Context.getConceptService().getConceptByReference(reference); + return Context.getConceptService().getConcept(Integer.valueOf(s)); } catch (Exception e) { // pass diff --git a/pom.xml b/pom.xml index c3ab2cc4fae0..e6c7391cd2fe 100644 --- a/pom.xml +++ b/pom.xml @@ -942,7 +942,7 @@ org.apache.maven.plugins maven-dependency-plugin - 3.7.0 + 3.6.1 org.apache.maven.plugins From 5ad79d328def0d2c6720eef16bc4d706df0eb269 Mon Sep 17 00:00:00 2001 From: Wikum Weerakutti Date: Thu, 20 Jun 2024 17:32:06 +0530 Subject: [PATCH 20/20] Removing random changes --- api/src/main/java/org/openmrs/Concept.java | 4 ++-- pom.xml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/org/openmrs/Concept.java b/api/src/main/java/org/openmrs/Concept.java index ecd9833cd7eb..8a63b0e1b4b0 100644 --- a/api/src/main/java/org/openmrs/Concept.java +++ b/api/src/main/java/org/openmrs/Concept.java @@ -1436,9 +1436,9 @@ public List getPossibleValues() { * @see org.openmrs.Attributable#hydrate(java.lang.String) */ @Override - public Concept hydrate(String s) { + public Concept hydrate(String reference) { try { - return Context.getConceptService().getConcept(Integer.valueOf(s)); + return Context.getConceptService().getConceptByReference(reference); } catch (Exception e) { // pass diff --git a/pom.xml b/pom.xml index e6c7391cd2fe..c3ab2cc4fae0 100644 --- a/pom.xml +++ b/pom.xml @@ -942,7 +942,7 @@ org.apache.maven.plugins maven-dependency-plugin - 3.6.1 + 3.7.0 org.apache.maven.plugins