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

wikumChamith · 2024-03-25T13:33:11Z

Description of what I changed

Adding @Authorized(PrivilegeConstants.GET_GLOBAL_PROPERTIES) annotation to throw an authentication exception if a user is not authenticated when requesting a global property. Used Context.addProxyPrivilege("Get Global Properties") to extend the privilege to anonymous users in tests and before login.

Issue I worked on

see https://openmrs.atlassian.net/browse/TRUNK-6203

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

dkayiwa · 2024-03-27T14:25:31Z

@wikumChamith when you run these changes with the reference application modules, were you able to log in and load the main landing dashboard?

wikumChamith · 2024-03-28T07:46:02Z

@dkayiwa Yes, Everything seems fine on my end.

dkayiwa · 2024-03-28T08:48:55Z

@wikumChamith did you accidentally leave out the setGlobalProperty() method?

wikumChamith · 2024-03-28T09:50:04Z

@dkayiwa I updated the PR.

dkayiwa · 2024-03-28T12:57:13Z

@wikumChamith are you still able to run the reference application even with that change without getting errors?

wikumChamith · 2024-03-28T13:24:42Z

@dkayiwa Yes, Are you getting any errors?

dkayiwa · 2024-03-28T13:48:18Z

@wikumChamith have you also tested and confirmed that, with your changes, non logged in users can no longer access global properties as per the ticket requirements? In other words, anonymous users should get some sort of access denied errors when they try to access this: http://localhost:8080/openmrs/ws/rest/v1/systemsetting

wikumChamith · 2024-03-30T15:49:35Z

@dkayiwa, I've made updates to the PR. Additionally, I've created pull requests for the modules to enable anonymous users to access the login page.

openmrs-module-uiframework : openmrs/openmrs-module-uiframework#77
openmrs-module-referenceapplication : openmrs/openmrs-module-referenceapplication#104
openmrs-module-legacyui : openmrs/openmrs-module-legacyui#190

Regarding the openmrs-module-owa, I encountered an issue. We need to add the 'Get Global Properties' privilege to allow anonymous users to access the login page. However, the method in question (OwaFilter.java#L39) gets called by all requests. Adding Context.addProxyPrivilege(GET_GLOBAL_PROPERTIES) here would grant the privilege to all anonymous requests.

One potential solution I could think of is using request.getHeader("referer") to add the privilege only to requests originating from the login page. Do you have any alternative suggestions for addressing this issue?"

dkayiwa · 2024-03-31T23:04:11Z

@wikumChamith how about if we intentionally expect this to fail until when accessed by a logged in user?

wikumChamith · 2024-04-01T09:13:06Z

@dkayiwa main issue here is users won't be able to access the login page because of that.

dkayiwa · 2024-04-04T14:05:49Z

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

@@ -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",


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

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

api/src/test/java/org/openmrs/test/jupiter/StartModuleExecutionListener.java

api/src/test/java/org/openmrs/test/StartModuleExecutionListener.java

api/src/test/java/org/openmrs/api/db/ContextDAOTest.java

api/src/test/java/org/openmrs/api/UserServiceTest.java

wikumChamith · 2024-05-09T13:41:46Z

@dkayiwa I have created another PR for this using a different approach: #4617

What method do you suggest?

dkayiwa · 2024-05-09T13:53:16Z

@dkayiwa I have created another PR for this using a different approach: #4617

What method do you suggest?

I think the approach in this pull request is simpler. Don't you think so?

dkayiwa · 2024-05-09T14:01:33Z

api/src/main/java/org/openmrs/util/LocaleUtility.java

@@ -57,6 +57,7 @@ public static Locale getDefaultLocale() {
 		if (defaultLocaleCache == null) {
 			if (Context.isSessionOpen()) {
 				try {
+					Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);


Do we really need this? Who calls it in the startup process?

dkayiwa · 2024-05-09T14:14:37Z

web/src/main/java/org/openmrs/web/filter/GZIPFilter.java

 	}

 	/**
 	 * Returns true if path matches pattern in gzip.acceptCompressedRequestsForPaths property
 	 */
 	private boolean isCompressedRequestForPathAccepted(String path) {
 		try {
+			Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);


Do we really need this? Who calls it in the startup process?

dkayiwa · 2024-05-09T14:45:07Z

web/src/main/java/org/openmrs/web/filter/GZIPFilter.java

@@ -124,9 +125,9 @@ private boolean isGZIPEnabled() {
 		}

 		try {
+			Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);


We could also just catch the APIAuthenticationException here and log it, before returning false, in order to reduce the need to add proxy privileges. This is based on the fact that it is not a big deal for a not yet logged in user to miss gzip compression. They can have it after login.

@wikumChamith i am changing my mind on this. I think it would be better to add the proxy privilege to avoid these unnecessary errors before login. :)

dkayiwa · 2024-05-09T17:54:01Z

api/src/test/java/org/openmrs/annotation/StartModuleAnnotationTest.java

 import org.openmrs.test.StartModule;
+import org.openmrs.test.jupiter.BaseContextSensitiveTest;
+import org.openmrs.util.RoleConstants;


Do we need to make any changes in this file?

dkayiwa · 2024-05-09T18:33:34Z

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"/>


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.

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.

dkayiwa · 2024-05-13T20:29:42Z

api/src/test/java/org/openmrs/api/UserServiceTest.java

@@ -473,8 +475,14 @@ public void changePassword_shouldMatchOnIncorrectlyHashedSha1StoredPassword() {
 		executeDataSet(XML_FILENAME);
 		Context.logout();
 		Context.authenticate("incorrectlyhashedSha1", "test");
-
-		userService.changePassword("test", "Tester12");
+		try {


Do we really need this try finally?

dkayiwa · 2024-05-13T20:31:09Z

api/src/test/java/org/openmrs/api/UserServiceTest.java

@@ -515,8 +523,13 @@ public void changePassword_shouldMatchOnCorrectlyHashedSha1StoredPassword() {
 		executeDataSet(XML_FILENAME);
 		Context.logout();
 		Context.authenticate("correctlyhashedSha1", "test");
-
-		userService.changePassword("test", "Tester12");
+		try {


Do we really need this try finally? And the ones below?

dkayiwa · 2024-06-11T09:25:04Z

Try create a new server.

wikumChamith · 2024-06-11T13:58:48Z

@dkayiwa I tested on both Reference Application 2.14.0-SNAPSHOT and 2.13.0. Looks like the issue is with the new SPA module. I'll look into this.

dkayiwa · 2024-06-11T14:34:53Z

I'll look into this.

Awesome! Looking forward to the fix. 😊

wikumChamith · 2024-06-19T14:18:30Z

@dkayiwa, I am encountering an issue when running the O2 reference application with these changes. Upon launching the server, I face a browser error stating:

localhost redirected you too many times.: ERR_TOO_MANY_REDIRECTS

Apart from this, the only backend error I saw during redirects:

INFO - uncaughtException_jsp._jspService(515) |2024-06-19T19:01:24,921| Exception was thrown by not authenticated user

It looks like the issue is related to the openmrs-module-referenceapplication as the legacy-ui login page is accessible without this module. I also tried giving proxy privileges in a few instances within the reference application module where global properties were accessed but that didn't resolve the issue.

Any insights on how to resolve this are highly appreciated.

dkayiwa · 2024-06-19T14:37:38Z

@wikumChamith does this problem happen only with your modified version of openmrs-core?

wikumChamith · 2024-06-19T14:42:42Z

@wikumChamith does this problem happen only with your modified version of openmrs-core?

Yeah. This only occurs on TRUNK-6203

wikumChamith · 2024-06-19T16:33:55Z

@dkayiwa This fixes the issue: openmrs/openmrs-module-referenceapplication#105

Previously this didn't work for me because of another issue on my side.

dkayiwa · 2024-06-20T11:38:07Z

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

 		try {
-			return Context.getConceptService().getConcept(Integer.valueOf(s));


@wikumChamith is this supposed to be part of this pull request?

Where is this from 😅 ?

You better find out. 😊

dkayiwa · 2024-06-20T11:39:58Z

pom.xml

@@ -942,7 +942,7 @@
 			<plugin>
 				<groupId>org.apache.maven.plugins</groupId>
 				<artifactId>maven-dependency-plugin</artifactId>
-				<version>3.6.1</version>
+				<version>3.7.0</version>


Is this supposed to be part of this pul request?

wikumChamith · 2024-06-20T11:52:11Z

@dkayiwa I think those two changes are from the rebase. They are similar to the current master:

Concept#hydrate

pom.xml

dkayiwa · 2024-06-20T11:53:14Z

I do not care where they came from. All i want is not to see them here. 😊

wikumChamith · 2024-06-20T12:02:56Z

@dkayiwa I removed those changes.

wikumChamith · 2024-06-21T14:27:26Z

@dkayiwa I updated the PR.

* 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

wikumChamith force-pushed the TRUNK-6203 branch from 50131c7 to b01e48d Compare March 28, 2024 09:49

wikumChamith force-pushed the TRUNK-6203 branch 2 times, most recently from 82724fd to 37b5a03 Compare March 30, 2024 15:04

dkayiwa reviewed Apr 4, 2024

View reviewed changes