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..6c092ea2d2f4 100644 --- a/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java +++ b/api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java @@ -9,12 +9,12 @@ */ package org.openmrs.annotation; -import static org.junit.jupiter.api.Assertions.assertNotNull; - import org.junit.jupiter.api.Test; import org.openmrs.api.context.Context; -import org.openmrs.test.jupiter.BaseContextSensitiveTest; import org.openmrs.test.StartModule; +import org.openmrs.test.jupiter.BaseContextSensitiveTest; + +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..9227815970ea 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; @@ -554,7 +552,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 +586,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 +605,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 +621,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 +658,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 +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 @@ -681,6 +678,9 @@ 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 +735,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 +752,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 f8246682c4e2..fe27de4a187b 100644 --- a/api/src/test/java/org/openmrs/api/UserServiceTest.java +++ b/api/src/test/java/org/openmrs/api/UserServiceTest.java @@ -268,6 +268,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 @@ -301,6 +302,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 @@ -335,8 +337,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 @@ -502,9 +504,8 @@ 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 } @@ -544,9 +545,8 @@ public void changePassword_shouldMatchOnCorrectlyHashedSha1StoredPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("correctlyhashedSha1", "test"); - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); - Context.logout(); // so that the next test reauthenticates } @@ -572,9 +572,8 @@ public void changePassword_shouldMatchOnSha512HashedPassword() { executeDataSet(XML_FILENAME); Context.logout(); Context.authenticate("userWithSha512Hash", "test"); - + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); userService.changePassword("test", "Tester12"); - Context.logout(); // so that the next test reauthenticates } @@ -1520,9 +1519,9 @@ 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"); - + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); Context.authenticate(user.getUsername(), "userServiceTest2"); } @@ -1614,7 +1613,9 @@ public void changePasswordUsingActivationKey_shouldUpdatePasswordIfActivationKey final String PASSWORD = "Admin123"; Context.authenticate(createdUser.getUsername(), "Openmr5xy"); + 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 688c5cfbf3f3..540df393407e 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; /** @@ -279,6 +280,7 @@ public void authenticate_shouldPassRegressionTestFor1580() { Context.logout(); // first we fail a login attempt + Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); try { dao.authenticate("admin", "not the right password"); fail("Not sure why this username/password combo worked"); @@ -309,7 +311,13 @@ 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"); + Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); + }); + + 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..ce724b21e748 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 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 @@ - - + +