From 7f46f41dd9d458631e58dfc7806fb74be48f0565 Mon Sep 17 00:00:00 2001 From: Wikum Chamith Date: Wed, 20 Mar 2024 17:45:11 +0530 Subject: [PATCH] TRUNK-6203: Global properties access should be privileged --- .../openmrs/api/AdministrationService.java | 9 ++++++++ .../annotation/StartModuleAnnotationTest.java | 8 ++++--- .../openmrs/aop/AuthorizationAdviceTest.java | 2 +- .../api/AdministrationServiceTest.java | 22 ++++++++++--------- .../java/org/openmrs/api/UserServiceTest.java | 10 +++++++-- .../org/openmrs/api/db/ContextDAOTest.java | 8 ++++++- .../test/StartModuleExecutionListener.java | 3 +++ .../jupiter/StartModuleExecutionListener.java | 3 +++ ...nistrationServiceTest-globalproperties.xml | 4 ++-- .../module/web/filter/ModuleFilterChain.java | 3 +++ .../org/openmrs/web/filter/GZIPFilter.java | 3 +++ .../org/openmrs/web/filter/StartupFilter.java | 1 + 12 files changed, 57 insertions(+), 19 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/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..b36ad15e06a8 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(); @@ -588,9 +588,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,6 +607,7 @@ public void getGlobalProperty_shouldReturnGlobalPropertyIfUserIsAllowedToView() Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getViewPrivilege()); Context.getAuthenticatedUser().addRole(role); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); assertNotNull(adminService.getGlobalProperty(property.getProperty())); } @@ -625,8 +625,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())); } /** @@ -644,6 +644,7 @@ public void getGlobalPropertyObject_shouldReturnGlobalPropertyIfUserIsAllowedToV Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getViewPrivilege()); Context.getAuthenticatedUser().addRole(role); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); assertNotNull(adminService.getGlobalPropertyObject(property.getProperty())); } @@ -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())); } /** @@ -682,6 +683,7 @@ public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty Role role = Context.getUserService().getRole("Provider"); role.addPrivilege(property.getEditPrivilege()); Context.getAuthenticatedUser().addRole(role); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); adminService.updateGlobalProperty(property.getProperty(), "new-value"); String newValue = adminService.getGlobalProperty(property.getProperty()); @@ -735,7 +737,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 +754,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 1b3b1bc61263..63a3af981035 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -261,6 +261,7 @@ public void createUser_shouldNotAllowCreatingUserWithPrivilegeCurrentUserDoesNot userRole.setRole(RoleConstants.AUTHENTICATED); userRole.addPrivilege(new Privilege("Add Users")); currentUser.addRole(userRole); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); // 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 @@ -294,6 +295,7 @@ public void createUser_shouldNotAllowCreatingUserWithPrivilegesCurrentUserDoesNo userRole.setRole(RoleConstants.AUTHENTICATED); userRole.addPrivilege(new Privilege("Add Users")); currentUser.addRole(userRole); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); // 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 @@ -328,7 +330,7 @@ public void createUser_shouldNotAllowAssigningSuperUserRoleIfCurrentUserDoesNotH userRole.setRole(RoleConstants.AUTHENTICATED); userRole.addPrivilege(new Privilege("Add Users")); currentUser.addRole(userRole); - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); // 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 @@ -473,7 +475,7 @@ public void changePassword_shouldMatchOnIncorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("incorrectlyhashedSha1", "test"); - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); Context.logout(); // so that the next test reauthenticates @@ -515,6 +517,7 @@ public void changePassword_shouldMatchOnCorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("correctlyhashedSha1", "test"); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); @@ -543,6 +546,7 @@ public void changePassword_shouldMatchOnSha512HashedPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("userWithSha512Hash", "test"); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); @@ -1491,6 +1495,7 @@ public void changePasswordUsingSecretAnswer_shouldUpdatePasswordIfSecretIsCorrec User user = userService.getUser(6001); assertFalse(user.hasPrivilege(PrivilegeConstants.EDIT_USER_PASSWORDS)); Context.authenticate(user.getUsername(), "userServiceTest"); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePasswordUsingSecretAnswer("answer", "userServiceTest2"); @@ -1585,6 +1590,7 @@ public void changePasswordUsingActivationKey_shouldUpdatePasswordIfActivationKey final String PASSWORD = "Admin123"; Context.authenticate(createdUser.getUsername(), "Openmr5xy"); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePasswordUsingActivationKey(key, PASSWORD); 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..61b8fdedd7b8 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; /** @@ -277,6 +278,8 @@ public void authenticate_shouldAuthenticateWithIncorrectHashedPassword() { public void authenticate_shouldPassRegressionTestFor1580() { // logout after the base setup Context.logout(); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + // first we fail a login attempt try { @@ -309,7 +312,10 @@ public void authenticate_shouldPassRegressionTestFor1580() { // 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, () -> { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + dao.authenticate("admin", "test"); + }); } @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..96f5492cf92c 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; @@ -80,6 +81,7 @@ public void prepareTestInstance(TestContext testContext) throws Exception { if (!Context.isSessionOpen()) Context.openSession(); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); ModuleUtil.shutdown(); @@ -160,6 +162,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..3d2d6a8452f8 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,8 @@ public void prepareTestInstance(TestContext testContext) throws Exception { if (!Context.isSessionOpen()) Context.openSession(); + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + ModuleUtil.shutdown(); // load the omods that the dev defined for this class 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/module/web/filter/ModuleFilterChain.java b/web/src/main/java/org/openmrs/module/web/filter/ModuleFilterChain.java index c3ab985c8254..44a94ab45c37 100644 --- a/web/src/main/java/org/openmrs/module/web/filter/ModuleFilterChain.java +++ b/web/src/main/java/org/openmrs/module/web/filter/ModuleFilterChain.java @@ -9,6 +9,9 @@ */ package org.openmrs.module.web.filter; +import org.openmrs.api.context.Context; +import org.openmrs.util.PrivilegeConstants; + import java.io.IOException; import java.util.Collection; import java.util.Iterator; 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..a018724b317f 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,8 +125,10 @@ private boolean isGZIPEnabled() { } try { + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); String gzipEnabled = Context.getAdministrationService().getGlobalProperty( OpenmrsConstants.GLOBAL_PROPERTY_GZIP_ENABLED, ""); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); cachedGZipEnabledFlag = Boolean.valueOf(gzipEnabled); return cachedGZipEnabledFlag; diff --git a/web/src/main/java/org/openmrs/web/filter/StartupFilter.java b/web/src/main/java/org/openmrs/web/filter/StartupFilter.java index ef2e65df6b19..771d727623db 100644 --- a/web/src/main/java/org/openmrs/web/filter/StartupFilter.java +++ b/web/src/main/java/org/openmrs/web/filter/StartupFilter.java @@ -55,6 +55,7 @@ import org.openmrs.logging.OpenmrsLoggingUtil; import org.openmrs.util.LocaleUtility; import org.openmrs.util.OpenmrsUtil; +import org.openmrs.util.PrivilegeConstants; import org.openmrs.web.Listener; import org.openmrs.web.WebConstants; import org.openmrs.web.filter.initialization.InitializationFilter;