openmrs-core
openmrs-core copied to clipboard
openmrs
TRUNK-6203: Global properties access should be privileged
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 :)
-
[x] 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 -
[x] 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 -
[x] I ran
mvn clean packageright before creating this pull request and added all formatting changes to my commit.No? -> execute above command
-
[x] 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.
-
[x] My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master
@wikumChamith when you run these changes with the reference application modules, were you able to log in and load the main landing dashboard?
@wikumChamith did you accidentally leave out the setGlobalProperty() method?
@wikumChamith are you still able to run the reference application even with that change without getting errors?
@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
@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 : https://github.com/openmrs/openmrs-module-uiframework/pull/77 openmrs-module-referenceapplication : https://github.com/openmrs/openmrs-module-referenceapplication/pull/104 openmrs-module-legacyui : https://github.com/openmrs/openmrs-module-legacyui/pull/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?"
@wikumChamith how about if we intentionally expect this to fail until when accessed by a logged in user?
@dkayiwa main issue here is users won't be able to access the login page because of that.
@dkayiwa I have created another PR for this using a different approach: https://github.com/openmrs/openmrs-core/pull/4617
What method do you suggest?
@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, it's crucial to use try-finally when using the addProxyPrivilege method. Without it, we risk being unable to remove the privilege in case of an exception. This approach aligns with the method's Javadoc comment as well
https://github.com/openmrs/openmrs-core/blob/0060a1d80b91658bb2fbe381daff4e0a5919392f/api/src/main/java/org/openmrs/api/context/UserContext.java#L224-L239
@dkayiwa, it's crucial to use try-finally when using the addProxyPrivilege method. Without it, we risk being unable to remove the privilege in case of an exception. This approach aligns with the method's Javadoc comment as well
https://github.com/openmrs/openmrs-core/blob/0060a1d80b91658bb2fbe381daff4e0a5919392f/api/src/main/java/org/openmrs/api/context/UserContext.java#L224-L239
Since this is a test where you are sure whether an exception is thrown or not, do you still need it?
@dkayiwa, it's crucial to use try-finally when using the addProxyPrivilege method. Without it, we risk being unable to remove the privilege in case of an exception. This approach aligns with the method's Javadoc comment as well https://github.com/openmrs/openmrs-core/blob/0060a1d80b91658bb2fbe381daff4e0a5919392f/api/src/main/java/org/openmrs/api/context/UserContext.java#L224-L239
Since this is a test where you are sure whether an exception is thrown or not, do you still need it?
Shouldn't we be consistent across the code base?
Shouldn't we be consistent across the code base?
Not where it does not make sense. 😊
@wikumChamith run these changes with all the O3 modules. Then make pull requests for any of those modules that need proxy privileges to access global properties.
@wikumChamith run these changes with all the O3 modules. Then make pull requests for any of those modules that need proxy privileges to access global properties.
@dkayiwa The changes in these two PRs enable us to start O3. However, there is an issue when starting a few modules, and I am currently looking into it.
spa module: https://github.com/openmrs/openmrs-module-spa/pull/58 owa module: https://github.com/openmrs/openmrs-module-owa/pull/78
I only tested this on an older version due to the issue with creating the latest O3 snapshot with the SDK.
@wikumChamith ping me after you are done testing this with the O2 reference application and confirmed that all is well.
@dkayiwa O2 reference application does not load with these changes. I am getting the following error on the browser.
"ERR_TOO_MANY_REDIRECTS"
It is hard to pinpoint what is causing it from the serverside logs but I noticed the following error when refreshing the page.
INFO - uncaughtException_jsp._jspService(515) |2024-06-11T12:34:30,315| Exception was thrown by not authenticated user
INFO - StatisticalLoggingSessionEventListener.end(258) |2024-06-11T12:34:30,316| Session Metrics {
0 nanoseconds spent acquiring 0 JDBC connections;
0 nanoseconds spent releasing 0 JDBC connections;
0 nanoseconds spent preparing 0 JDBC statements;
0 nanoseconds spent executing 0 JDBC statements;
0 nanoseconds spent executing 0 JDBC batches;
0 nanoseconds spent performing 0 L2C puts;
0 nanoseconds spent performing 0 L2C hits;
0 nanoseconds spent performing 0 L2C misses;
0 nanoseconds spent executing 0 flushes (flushing a total of 0 entities and 0 collections);
0 nanoseconds spent executing 0 partial-flushes (flushing a total of 0 entities and 0 collections)
}
Full logs: https://pastebin.com/sAX0c4BT
What do you think that might be causing this?
Which exact version of the O2 reference application?
@dkayiwa I tried with the snapshot.
@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, 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.
@wikumChamith does this problem happen only with your modified version of openmrs-core?
@wikumChamith does this problem happen only with your modified version of openmrs-core?
Yeah. This only occurs on TRUNK-6203
@dkayiwa This fixes the issue: https://github.com/openmrs/openmrs-module-referenceapplication/pull/105
Previously this didn't work for me because of another issue on my side.