-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
TRUNK-6203: Global properties access should be privileged #4562
base: master
Are you sure you want to change the base?
Conversation
if (propertyName == null) { | ||
return null; | ||
} | ||
try { | ||
Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); | ||
if (!Context.isAuthenticated()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The authentication check is automatically done by the framework. So not need for all this logic here. Not so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkayiwa you must be right. Let me remove it
Hi @dkayiwa , updates made on this PR, kindly review! |
Whenever you are ready for review, you need to change the pull request from draft. |
Thank you @dkayiwa, I have done as you suggested. |
Are you able to successfully run openmrs-core with these changes? |
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 |
Can you run core, with these changes, together with all the reference application 2.x modules? |
Yes! However, there is something I have just discovered. When I run the core with |
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? |
@@ -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 | |||
*/ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this should also have an @Authorized
annotation, right? (I realize that it calls the other getGlobalProperty()
, but that's an implementation detail.
if (propertyName == null) { | ||
return null; | ||
} | ||
try { | ||
Context.addProxyPrivilege(PrivilegeConstants.GET_GLOBAL_PROPERTIES); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
} | ||
|
||
String[] anonymousArray = { "owa.appBaseUrl", "login.url", "spa.baseUrl", "timezone.conversions", "default_theme", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these should be moved to the top of the file with the remainder of the properties.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
}
@ibacher wouldn't this work best if the list was quite big? |
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. |
Unfortunately, the error reappeared after an hour! Wow cc @dkayiwa |
Welcome to coding. 😊 |
Hi @dkayiwa, addresshierarchy module uses |
Use the actual string value |
Thank you, I had tried that earlier, but the problem still was there! Maybe I had done something wrong. Let me try again |
HI @dkayiwa, here is the PR under addresshierarchy project openmrs/openmrs-module-addresshierarchy#41 |
531980f
to
cb6d2d7
Compare
Hi @dkayiwa I am kindly requesting for the review of this PR |
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 andadded 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