TRUNK-6203: Global properties access should be privileged (openmrs#4601)

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* TRUNK-6203: Global properties access should be privileged

* Removing random changes

* Removing random changes

api/src/main/java/org/openmrs/api/AdministrationService.java

@@ -56,6 +56,7 @@ public interface AdministrationService extends OpenmrsService {
 	 * <strong>Should</strong> find object given valid uuid
 	 * <strong>Should</strong> 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 {
 	 * <strong>Should</strong> get property value given valid property name
 	 * <strong>Should</strong> 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 {
 	 * <strong>Should</strong> return default value if property name does not exist
 	 * <strong>Should</strong> 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 <code>propertyName</code>
 	 * <strong>Should</strong> 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
 	 * <strong>Should</strong> return all relevant global properties in the database
 	 */
+	@Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES)
 	public List<GlobalProperty> getGlobalPropertiesByPrefix(String prefix);
 
 	/**
@@ -135,6 +140,7 @@ public interface AdministrationService extends OpenmrsService {
 	 * @since 1.6
 	 * <strong>Should</strong> return all relevant global properties in the database
 	 */
+	@Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES)
 	public List<GlobalProperty> getGlobalPropertiesBySuffix(String suffix);
 
 	/**
@@ -189,6 +195,7 @@ public interface AdministrationService extends OpenmrsService {
 	 * <strong>Should</strong> overwrite global property if exists
 	 * <strong>Should</strong> 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 {
 	 * <strong>Should</strong> fail if global property being updated does not already exist
 	 * <strong>Should</strong> 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> T getGlobalPropertyValue(String propertyName, T defaultValue);
 
 	/**

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 {

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());

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<GlobalProperty> 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());

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);
 
 	}

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

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

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

api/src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml

@@ -21,8 +21,8 @@
   <global_property property="valid.integer" property_value="1234" uuid="b7225607-d0c4-4e44-8be5-31e1ac7e1fda"/>
   <global_property property="valid.double" property_value="1234.54" uuid="b7225607-d0c4-4e44-8be6-31e1ac7e1fda"/>
 
-	<privilege privilege="Some Privilege For View Global Properties" description="This is a test privilege for view global properties" uuid="d979d066-15e6-467c-9d4b-cb575ef12345"/>
-	<privilege privilege="Some Privilege For Edit Global Properties" description="This is a test privilege for edit global properties" uuid="d979d066-15e6-467c-9d4b-cb575ef54321"/>
+	<privilege privilege="Get Global Properties" description="This is a test privilege for view global properties" uuid="d979d066-15e6-467c-9d4b-cb575ef12345"/>
+	<privilege privilege="Manage Global Properties" description="This is a test privilege for edit global properties" uuid="d979d066-15e6-467c-9d4b-cb575ef54321"/>
 	<privilege privilege="Some Privilege For Delete Global Properties" description="This is a test privilege for delete global properties" uuid="d979d066-15e6-467c-9d4b-cb575ef67890"/>
 	<users user_id="5506" person_id="501" system_id="7-5" username="test_user" password="4a1750c8607d0fa237de36c6305715c223415189" salt="c788c6ad82a157b712392ca695dfcf2eed193d7f" secret_question="" creator="1" date_created="2008-08-15 15:57:09.0" changed_by="1" date_changed="2008-08-18 11:51:56.0" retired="false" retire_reason="" uuid="06d05314-e132-11de-babe-001e37123456"/>
 </dataset>