WrenSecurity/wrends

HTTP endpoints do not return full JSON response

Closed this issue · 5 comments

Summary

HTTP endpoints do not return full JSON response.

$ curl -u admin:password http://localhost:8080/admin
{"_rev":"0000000054c141ca"
$ curl -u admin:password http://localhost:8080/api
{"_rev":"0000000054c141ca"

Steps To Reproduce ("Repro Steps")

  1. Enable HTTP Connector Handler
  2. Send GET requests to HTTP endpoints

Expected Result (Behavior You Expected to See)

Full readable JSON response.

Actual Result (Behavior You Saw)

Incomplete JSON response.

Environment

  • Product version: 4.0.0-RC2
  • JRE version: openjdk 17.0.6

I have tried to replicate stuff and ended up with completely broken installation that refuses to start due to this error:

[13/Apr/2023:10:09:12 +0200] category=org.opends.messages.external severity=ERROR msgID=1 msg=Trying to redefine version: 0.0 exception=IllegalArgumentException: Trying to redefine version: 0.0 (CrestApiProducer.java:129 CrestApiProducer.java:45 AbstractRouter.java:369 AbstractRouter.java:342 AbstractRouter.java:290 Resource.java:687 SubResource.java:221 SubResource.java:141 AbstractRouter.java:361 AbstractRouter.java:342 AbstractRouter.java:290 DescribableRequestHandler.java:109 DescribableRequestHandler.java:40 AbstractRouter.java:361 AbstractRouter.java:342 AbstractRouter.java:290 FilterChain.java:273 FilterChain.java:38 InternalConnection.java:118 InternalConnection.java:26 HttpAdapter.java:849 ...)

Seems like there are more things broken there.

There are multiple issues at play. However the original issue is that for the root DSE result object is resolved as null by ObjectPropertyMapper.

I am not sure if such behavior is valid - i.e. returning null response content with proper identifier. This leads to NPE when actually serializing the response at RequestRunner.

Seems like the issue is with RequestRunner at a first glance. Maybe it should be able to handle / serialize null values?

Alternatively the ObjectPropertyMapper might be changed to return empty object instead of null:

--- a/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/ObjectPropertyMapper.java
+++ b/opendj-rest2ldap/src/main/java/org/forgerock/opendj/rest2ldap/ObjectPropertyMapper.java
@@ -318,7 +318,7 @@ public final class ObjectPropertyMapper extends PropertyMapper {
                            public JsonValue apply(final List<Pair<String, JsonValue>> value) {
                                if (value.isEmpty()) {
                                    // No subordinate attributes, so omit the entire JSON object from the resource.
-                                   return null;
+                                   return new JsonValue(Collections.emptyMap());
                                } else {
                                    // Combine the sub-attributes into a single JSON object.
                                    final Map<String, Object> result = new LinkedHashMap<>(value.size());

I have tried to replicate stuff and ended up with completely broken installation that refuses to start due to this error:

[13/Apr/2023:10:09:12 +0200] category=org.opends.messages.external severity=ERROR msgID=1 msg=Trying to redefine version: 0.0 exception=IllegalArgumentException: Trying to redefine version: 0.0 (CrestApiProducer.java:129 CrestApiProducer.java:45 AbstractRouter.java:369 AbstractRouter.java:342 AbstractRouter.java:290 Resource.java:687 SubResource.java:221 SubResource.java:141 AbstractRouter.java:361 AbstractRouter.java:342 AbstractRouter.java:290 DescribableRequestHandler.java:109 DescribableRequestHandler.java:40 AbstractRouter.java:361 AbstractRouter.java:342 AbstractRouter.java:290 FilterChain.java:273 FilterChain.java:38 InternalConnection.java:118 InternalConnection.java:26 HttpAdapter.java:849 ...)

This has been introduced in #7 by producing more than one descriptor for the same path. Will have to go through that PR and API description spec and decide what to do with that change.

I have spent quite a lot of time on this and came to the following conclusion:

I understand the intention behind the original change performed in #7 - i.e. having read-only resources automatically produce API descriptors. However the overall change is simply not compatible with the resource <-> path + service mapping. I don't see any simple way to fix this without making some compromise with unwanted side effects.

I am going to create PR that will revert that change so that it does not block proper release.

Changes merged in #79 .