-
Notifications
You must be signed in to change notification settings - Fork 237
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-6218: Enable XStream whitelisting on OpenMRS Core 2.7.0 and after #253
Conversation
@@ -115,4 +122,22 @@ public void serializeToStream(Object object, OutputStream out) { | |||
throw new IllegalStateException("Unsupported encoding", e); | |||
} | |||
} | |||
|
|||
private void setupXStreamSecurity(XStream xstream) throws SerializationException { | |||
try { |
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.
I feel that instead of checking for the running version of openmrs, it is this method implementation that needs to be fixed regardless of which openmrs version we are running.
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 there is an issue on the core that caused it not to throw the API exception. I made a fix for that: openmrs/openmrs-core#4843
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.
This does not necessarily swallow the exception, but results into a situation where, while refreshing the spring application context, getRegisteredComponent does not find a bean, hence throwing an exception whose constructor calls Context.getMessageSourceService()` resulting into a deadlock here: https://github.com/openmrs/openmrs-core/blob/2.6.9/api/src/main/java/org/openmrs/api/context/ServiceContext.java#L681
Closing this PR as the issue got resolved here: openmrs/openmrs-core#4843 |
@wikumChamith i have reverted the other one and merged yours which had better formatting 😊 |
@@ -85,6 +87,11 @@ public Object unmarshal(HierarchicalStreamReader reader, Object root) { | |||
xstream.registerConverter(new IndicatorConverter(mapper, converterLookup)); | |||
|
|||
xstream.registerConverter(new ReportDefinitionConverter(mapper, converterLookup)); | |||
|
|||
// Only setup XStreamSecurity only on versions that are after 2.7.0 | |||
if (new VersionComparator().compare(OpenmrsConstants.OPENMRS_VERSION, "2.7.0") >= 0) { |
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 do we need this check now?
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.
Are you planning to run the reporting module on only platform 2.7? 😊
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.
This check was to only set up XStream security on 2.7.0 and higher.
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.
If you do not have that check, you are going to end up with versions of openmrs core before 2.7 freezing on startup. Isn't that what you were fixing here? openmrs/openmrs-core@f0910d4
Or has your fix now confused you? 😊
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.
Because we did that fix we don't need this check now.
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.
How about those who are not yet ready to upgrade their platform versions?
No description provided.