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

Seremba · 2024-02-16T14:56:08Z

Description of what I changed

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-02-18T17:19:22Z

api/src/main/java/org/openmrs/api/impl/AdministrationServiceImpl.java

 if (propertyName == null) {
 return null;
 }
+ try {
+ Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);
+ if (!Context.isAuthenticated()) {


The authentication check is automatically done by the framework. So not need for all this logic here. Not so?

@dkayiwa you must be right. Let me remove it

Seremba · 2024-02-21T21:18:11Z

Hi @dkayiwa , updates made on this PR, kindly review!

dkayiwa · 2024-02-21T21:20:51Z

Whenever you are ready for review, you need to change the pull request from draft.

Seremba · 2024-02-21T21:25:56Z

Whenever you are ready for review, you need to change the pull request from draft.

Thank you @dkayiwa, I have done as you suggested.

dkayiwa · 2024-02-21T21:45:29Z

Are you able to successfully run openmrs-core with these changes?
If yes, can you also load the legacyui module and confirm that you can log in successfully?

Seremba · 2024-02-21T21:57:53Z

Yes, I am able to run openmrs-core successfully and I have been able to login into the system. With loading the legacy UI module, do you mean deleting it and uploading its omod file again? As far as I can see, it is already loaded cc @dkayiwa

dkayiwa · 2024-02-21T22:03:15Z

Can you run core, with these changes, together with all the reference application 2.x modules?

Seremba · 2024-02-21T22:14:23Z

Yes! However, there is something I have just discovered. When I run the core with mvn openmrs-sdk:run -DserverId=server things work perfectly well and I am able login into the system and interact with it. But after like 10 to 15 minutes, without doing anything on the system and when still logged in, I get this error on the server logs. It does not affect the UI at all, neither does it affect the openmrs-core. https://pastebin.com/vdCkt50T. When I interact with the system, like say I add a new patient, the error then disappears and everything works normally. The error does not affect any operations thus far but it is there. uhm! cc @dkayiwa

dkayiwa · 2024-02-22T12:49:02Z

Clone the addresshierarchy module and add the requested proxy privilege as in the error log. Then compile and update your reference application. Do the errors disappear?

ibacher · 2024-02-19T15:34:53Z

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

@@ -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
 */
+


Presumably this should also have an @Authorized annotation, right? (I realize that it calls the other getGlobalProperty(), but that's an implementation detail.

ibacher · 2024-02-19T15:35:55Z

api/src/main/java/org/openmrs/api/impl/AdministrationServiceImpl.java

 if (propertyName == null) {
 return null;
 }
+ try {
+ Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES);


Does this work? I would expect, in the actual app, that we've already verified that the context has this privilege, since it's required to get by the @Authorized annotation on the interface method.

ibacher

Design-wise, I guess a hard-coded list is ok, but it would be even better if modules could actually declare which GP's should be anonymously accessible.

ibacher · 2024-02-22T21:19:56Z

api/src/main/java/org/openmrs/api/impl/AdministrationServiceImpl.java

+
+ }
+
+ String[] anonymousArray = { "owa.appBaseUrl", "login.url", "spa.baseUrl", "timezone.conversions", "default_theme",


All of these should be moved to the top of the file with the remainder of the properties.

ibacher · 2024-02-22T21:26:02Z

api/src/main/java/org/openmrs/api/impl/AdministrationServiceImpl.java

+ String[] anonymousArray = { "owa.appBaseUrl", "login.url", "spa.baseUrl", "timezone.conversions", "default_theme",
+ "gzip.enabled", "default_locale" };
+
+ private final Set<String> anonymouslyAccessibleProperties = new HashSet<>(Arrays.asList(anonymousArray));


It seems kind of silly to create an array, cast it into a list and then a set, when we could just do:

private final static Set<String> anonymouslyAccessibleProperties = new HashSet<>(); static { Collections.addAll(anonymouslyAccessibleProperties, "owa.appBaseUrl", "login.url", "spa.baseUrl", "timezone.conversions", "default_theme", "gzip.enabled", "default_locale") }

Seremba · 2024-02-23T09:03:13Z

Design-wise, I guess a hard-coded list is ok, but it would be even better if modules could actually declare which GP's should be anonymously accessible.

@ibacher wouldn't this work best if the list was quite big?

Seremba · 2024-02-23T11:47:48Z

Clone the addresshierarchy module and add the requested proxy privilege as in the error log. Then compile and update your reference application. Do the errors disappear?

Hi @dkayiwa, do I have to make a PR for the changes in addresshierarchy module too? It has been an hour and the error doesn't seem to appear anymore.

dkayiwa · 2024-02-23T11:54:51Z

Clone the addresshierarchy module and add the requested proxy privilege as in the error log. Then compile and update your reference application. Do the errors disappear?

Hi @dkayiwa, do I have to make a PR for the changes in addresshierarchy module too? It has been an hour and the error doesn't seem to appear anymore.

Yes create a jira ticket under that project and raise a pull request.

Seremba · 2024-02-23T12:01:06Z

Unfortunately, the error reappeared after an hour! Wow cc @dkayiwa

dkayiwa · 2024-02-23T12:28:56Z

Unfortunately, the error reappeared after an hour! Wow cc @dkayiwa

Welcome to coding. 😊

Seremba · 2024-02-27T17:48:48Z

Hi @dkayiwa, addresshierarchy module uses <openMRSVersion>1.10.6</openMRSVersion> and it misses Get Global Properties in its PrivilegeConstants file. I can't add it. It is a read-only file. Is there a way you recommend going about this?

dkayiwa · 2024-02-27T17:50:20Z

Use the actual string value Get Global Properties instead of the constant.

Seremba · 2024-02-27T18:08:01Z

Thank you, I had tried that earlier, but the problem still was there! Maybe I had done something wrong. Let me try again

Seremba · 2024-02-29T14:52:22Z

Clone the addresshierarchy module and add the requested proxy privilege as in the error log. Then compile and update your reference application. Do the errors disappear?

Hi @dkayiwa, do I have to make a PR for the changes in addresshierarchy module too? It has been an hour and the error doesn't seem to appear anymore.

Yes create a jira ticket under that project and raise a pull request.

HI @dkayiwa, here is the PR under addresshierarchy project openmrs/openmrs-module-addresshierarchy#41

Seremba · 2024-03-30T22:17:06Z

@dkayiwa , @ibacher is there anything more I am still meant to do with this PR?

Seremba · 2024-05-07T08:04:52Z

Hi @dkayiwa I am kindly requesting for the review of this PR

dkayiwa reviewed Feb 18, 2024

View reviewed changes

Seremba force-pushed the TRUNK-6203 branch from aefa361 to ea611f1 Compare February 21, 2024 21:02

Seremba marked this pull request as ready for review February 21, 2024 21:24

ibacher reviewed Feb 22, 2024

View reviewed changes

Seremba force-pushed the TRUNK-6203 branch 2 times, most recently from 531980f to cb6d2d7 Compare March 15, 2024 04:15

TRUNK-6203: Global properties access should be privileged

15040d8

Seremba force-pushed the TRUNK-6203 branch from 531980f to 15040d8 Compare May 7, 2024 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Seremba commented Feb 16, 2024 •

edited

dkayiwa Feb 18, 2024

Seremba Feb 18, 2024

Seremba commented Feb 21, 2024

dkayiwa commented Feb 21, 2024

Seremba commented Feb 21, 2024 •

edited

dkayiwa commented Feb 21, 2024

Seremba commented Feb 21, 2024 •

edited

dkayiwa commented Feb 21, 2024

Seremba commented Feb 21, 2024 •

edited

dkayiwa commented Feb 22, 2024

ibacher Feb 19, 2024

ibacher Feb 19, 2024

ibacher left a comment

ibacher Feb 22, 2024

ibacher Feb 22, 2024

Seremba commented Feb 23, 2024

Seremba commented Feb 23, 2024

dkayiwa commented Feb 23, 2024

Seremba commented Feb 23, 2024

dkayiwa commented Feb 23, 2024

Seremba commented Feb 27, 2024

dkayiwa commented Feb 27, 2024

Seremba commented Feb 27, 2024

Seremba commented Feb 29, 2024

Seremba commented Mar 30, 2024

Seremba commented May 7, 2024


		}

		String[] anonymousArray = { "owa.appBaseUrl", "login.url", "spa.baseUrl", "timezone.conversions", "default_theme",

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

Are you sure you want to change the base?

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

Conversation

Seremba commented Feb 16, 2024 • edited

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

dkayiwa Feb 18, 2024

Choose a reason for hiding this comment

Seremba Feb 18, 2024

Choose a reason for hiding this comment

Seremba commented Feb 21, 2024

dkayiwa commented Feb 21, 2024

Seremba commented Feb 21, 2024 • edited

dkayiwa commented Feb 21, 2024

Seremba commented Feb 21, 2024 • edited

dkayiwa commented Feb 21, 2024

Seremba commented Feb 21, 2024 • edited

dkayiwa commented Feb 22, 2024

ibacher Feb 19, 2024

Choose a reason for hiding this comment

ibacher Feb 19, 2024

Choose a reason for hiding this comment

ibacher left a comment

Choose a reason for hiding this comment

ibacher Feb 22, 2024

Choose a reason for hiding this comment

ibacher Feb 22, 2024

Choose a reason for hiding this comment

Seremba commented Feb 23, 2024

Seremba commented Feb 23, 2024

dkayiwa commented Feb 23, 2024

Seremba commented Feb 23, 2024

dkayiwa commented Feb 23, 2024

Seremba commented Feb 27, 2024

dkayiwa commented Feb 27, 2024

Seremba commented Feb 27, 2024

Seremba commented Feb 29, 2024

Seremba commented Mar 30, 2024

Seremba commented May 7, 2024

Seremba commented Feb 16, 2024 •

edited

Seremba commented Feb 21, 2024 •

edited

Seremba commented Feb 21, 2024 •

edited

Seremba commented Feb 21, 2024 •

edited