Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 21 commits into from
Jun 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
da8d953
TRUNK-6203: Global properties access should be privileged
wikumChamith Mar 20, 2024
36add4c
TRUNK-6203: Global properties access should be privileged
wikumChamith May 11, 2024
4cc16f2
TRUNK-6203: Global properties access should be privileged
wikumChamith May 11, 2024
1a8b789
TRUNK-6203: Global properties access should be privileged
wikumChamith May 14, 2024
b4a7b9b
TRUNK-6203: Global properties access should be privileged
wikumChamith May 14, 2024
d0f3b9b
TRUNK-6203: Global properties access should be privileged
wikumChamith May 15, 2024
5a2f188
TRUNK-6203: Global properties access should be privileged
wikumChamith May 15, 2024
574c6da
TRUNK-6203: Global properties access should be privileged
wikumChamith May 15, 2024
566eb78
TRUNK-6203: Global properties access should be privileged
wikumChamith May 15, 2024
0085916
TRUNK-6203: Global properties access should be privileged
wikumChamith Mar 20, 2024
959ccfb
TRUNK-6203: Global properties access should be privileged
wikumChamith May 11, 2024
8440366
TRUNK-6203: Global properties access should be privileged
wikumChamith May 11, 2024
8bd9bd9
TRUNK-6203: Global properties access should be privileged
wikumChamith May 14, 2024
be1b829
TRUNK-6203: Global properties access should be privileged
wikumChamith May 14, 2024
081220d
TRUNK-6203: Global properties access should be privileged
wikumChamith May 15, 2024
de74e91
TRUNK-6203: Global properties access should be privileged
wikumChamith May 15, 2024
3c9d1e4
TRUNK-6203: Global properties access should be privileged
wikumChamith May 15, 2024
7d4fe94
TRUNK-6203: Global properties access should be privileged
wikumChamith May 15, 2024
eae0e9b
Merge remote-tracking branch 'origin/TRUNK-6203' into TRUNK-6203
wikumChamith Jun 14, 2024
c0912d9
Removing random changes
wikumChamith Jun 20, 2024
5ad79d3
Removing random changes
wikumChamith Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions api/src/main/java/org/openmrs/api/AdministrationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

/**
Expand Down Expand Up @@ -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);

/**
Expand All @@ -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);

/**
Expand All @@ -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);

/**
Expand All @@ -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);

/**
Expand All @@ -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);

/**
Expand Down Expand Up @@ -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);

/**
Expand All @@ -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);

/**
Expand Down Expand Up @@ -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);

/**
Expand Down
6 changes: 3 additions & 3 deletions api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion api/src/test/java/org/openmrs/aop/AuthorizationAdviceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
26 changes: 13 additions & 13 deletions api/src/test/java/org/openmrs/api/AdministrationServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind explaining why you changed the contents of String.format()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format of the exception message changes after adding the authorization.

property.getViewPrivilege(), property.getProperty()));
APIAuthenticationException exception = assertThrows(APIAuthenticationException.class, () -> adminService.getGlobalProperty(property.getProperty()));
assertEquals(exception.getMessage(), String.format("Privileges required: %s", property.getViewPrivilege()));
}

/**
Expand All @@ -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()));
}

Expand All @@ -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()));
}

/**
Expand Down Expand Up @@ -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()));
}

/**
Expand All @@ -673,6 +669,7 @@ public void updateGlobalProperty_shouldFailIfUserIsNotAllowedToEditGlobalPropert
public void updateGlobalProperty_shouldUpdateIfUserIsAllowedToEditGlobalProperty() {
executeDataSet(ADMIN_INITIAL_DATA_XML);
GlobalProperty property = getGlobalPropertyWithEditPrivilege();
GlobalProperty globalPropertyWithViewPrivilege = getGlobalPropertyWithViewPrivilege();
Copy link
Member

@dkayiwa dkayiwa May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why get this from here when we are going to use it way below. It would be great to fetch it closest to the point of usage. What do you think?

Copy link
Member Author

@wikumChamith wikumChamith May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows us to call the getGlobalPropertyWithViewPrivilege method without adding the proxy privilege to the user. Otherwise, we'll have to add proxy privilege before calling this method.

assertEquals("anothervalue", property.getPropertyValue());

// authenticate new user without privileges
Expand All @@ -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");
Expand Down Expand Up @@ -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());
Expand All @@ -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());
Expand Down
19 changes: 10 additions & 9 deletions api/src/test/java/org/openmrs/api/UserServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -494,9 +496,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
}

Expand Down Expand Up @@ -536,9 +537,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
}

Expand All @@ -564,9 +564,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
}

Expand Down Expand Up @@ -1512,9 +1511,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");
}

Expand Down Expand Up @@ -1606,7 +1605,9 @@ public void changePasswordUsingActivationKey_shouldUpdatePasswordIfActivationKey

final String PASSWORD = "Admin123";
Context.authenticate(createdUser.getUsername(), "Openmr5xy");
Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);
wikumChamith marked this conversation as resolved.
Show resolved Hide resolved
userService.changePasswordUsingActivationKey(key, PASSWORD);
Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);
Context.authenticate(createdUser.getUsername(), PASSWORD);

}
Expand Down
10 changes: 9 additions & 1 deletion api/src/test/java/org/openmrs/api/db/ContextDAOTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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);
wikumChamith marked this conversation as resolved.
Show resolved Hide resolved
dao.authenticate("admin", "test");
Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After removing the proxy privilege above, do we again need to remove it below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need to remove it again. The code inside the lambda function only adds or removes the value to that context. I tested this using Context.hasPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES)

});

Context.removeProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);
}

@Test
Expand Down
5 changes: 5 additions & 0 deletions api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like this. It is too permissive. Why do we need to add the proxy privilege for the entire module startup process?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without adding this we get test errors similar to this:

[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.096 s <<< FAILURE! -- in org.openmrs.annotation.OpenmrsProfileExcludeFilterWithModulesJUnit4Test
[ERROR] org.openmrs.annotation.OpenmrsProfileExcludeFilterWithModulesJUnit4Test.match_shouldNotIncludeBeanIfModuleIsStarted -- Time elapsed: 0 s <<< ERROR!
java.lang.RuntimeException: Database out of sync with code: Anonymous role does not exist
	at org.openmrs.api.context.UserContext.getAnonymousRole(UserContext.java:370)
	at org.openmrs.api.context.UserContext.hasPrivilege(UserContext.java:347)
	at org.openmrs.api.context.Context.hasPrivilege(Context.java:736)
	at org.openmrs.aop.AuthorizationAdvice.before(AuthorizationAdvice.java:73)
	at org.springframework.aop.framework.adapter.MethodBeforeAdviceInterceptor.invoke(MethodBeforeAdviceInterceptor.java:57)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186)
	at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:241)
	at com.sun.proxy.$Proxy181.getGlobalProperty(Unknown Source)
	at org.openmrs.module.ModuleFactory.getModulesThatShouldStart(ModuleFactory.java:291)
	at org.openmrs.module.ModuleFactory.startModules(ModuleFactory.java:236)
	at org.openmrs.module.ModuleUtil.startup(ModuleUtil.java:130)
	at org.openmrs.test.StartModuleExecutionListener.prepareTestInstance(StartModuleExecutionListener.java:94)
	at org.springframework.test.context.TestContextManager.prepareTestInstance(TestContextManager.java:248)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.createTest(SpringJUnit4ClassRunner.java:227)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner$1.runReflectiveCall(SpringJUnit4ClassRunner.java:289)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.methodBlock(SpringJUnit4ClassRunner.java:291)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:246)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:97)
	at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61)
	at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:70)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:190)

Module startup needs access to global properties when getting the module repository:

openmrs-core/api/src/main/java/org/openmrs/module/ModuleUtil.java

Lines 481 to 485 in e962f22

if (StringUtils.isBlank(folderName)) {
AdministrationService as = Context.getAdministrationService();
folderName = as.getGlobalProperty(ModuleConstants.REPOSITORY_FOLDER_PROPERTY,
ModuleConstants.REPOSITORY_FOLDER_PROPERTY_DEFAULT);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still does not mean that we give the privilege to the entire module startup. Not so? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only give the privilege in tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even then, we could mask bugs. We need to locate the specific places that need them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should add privileges within the ModuleFactory as it is not a test class.

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
Expand Down
6 changes: 6 additions & 0 deletions api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

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
Expand Down
4 changes: 2 additions & 2 deletions ...src/test/resources/org/openmrs/api/include/AdministrationServiceTest-globalproperties.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we should change this file. Can't we do without these changes? The tests that expect these values should still do so.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing this could lead to problems with tests. For an example let's consider the following test.

openmrs-core/api/src/test/java/org/openmrs/api/AdministrationServiceTest.java

Lines 636 to 649 in 7d70503

public void getGlobalPropertyObject_shouldReturnGlobalPropertyIfUserIsAllowedToView() {
executeDataSet(ADMIN_INITIAL_DATA_XML);
GlobalProperty property = getGlobalPropertyWithViewPrivilege();
// authenticate new user without privileges
Context.logout();
Context.authenticate(getTestUserCredentials());
// add required privilege to user
Role role = Context.getUserService().getRole("Provider");
role.addPrivilege(property.getViewPrivilege());
Context.getAuthenticatedUser().addRole(role);
assertNotNull(adminService.getGlobalPropertyObject(property.getProperty()));
}

If we keep the privilege name as "Some Privilege For View Global Properties," this test will fail with an error message stating:

org.openmrs.api.APIAuthenticationException: Privileges required: Get Global Properties

We could mitigate the error by adding Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);
However, this goes against the test's purpose: to validate that only users with the appropriate privilege can access global properties.

<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>
Loading