Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rest2LDAP CREST API descriptors ignore read-only collection sub-resources #6

Closed
GuyPaddock opened this issue Oct 25, 2017 · 2 comments
Closed

Rest2LDAP CREST API descriptors ignore read-only collection sub-resources #6

GuyPaddock opened this issue Oct 25, 2017 · 2 comments
Labels
🪲 bug

Comments

@GuyPaddock
Copy link

GuyPaddock commented Oct 25, 2017 •
edited
Loading

Summary

If a collection sub-resource defined in the Rest2LDAP endpoints JSON file is marked "isReadOnly": true, that endpoint does not appear at all in the API descriptor returned by CREST.

Affected Version

master (4.0.0-SNAPSHOT)

Steps to Reproduce

  1. Alter the example-v1.json file, and add the following under resourceTypes/example-v1/subResources (ensure you don't remove the existing users and groups sub-resource definitions):
                "all-users": {
                    "type": "collection",
                    "dnTemplate": "ou=people,dc=example,dc=com",
                    "resource": "frapi:opendj:rest2ldap:user:1.0",
                    "namingStrategy": {
                        "type": "clientDnNaming",
                        "dnAttribute": "uid"
                    },
                    "isReadOnly": true

2, Alter the Rest2LdapJsonConfiguratorTest, adding the line System.out.println(prettyPrint(api)); right after the parseJson(prettyPrint(api)); line.
3. Run the test.
4. Observe the console output from the test.

Expected results

The output includes the new all-users sub-resource, even though it's read-only. Something like the attached expected.json.

Actual results

The output omits all read-only collection sub-resources. See the attached actual.json.

Attachments

actual vs. expected JSON.zip
@GuyPaddock
Copy link
Author

GuyPaddock commented Oct 25, 2017 •
edited
Loading

At first, I thought this issue was just an oversight on ForgeRock's part, since org.forgerock.opendj.rest2ldap.ReadOnlyRequestHandler does not define an api() method. It actually looks like org.forgerock.opendj.rest2ldap.Resource.collectionApi() is setup to be able to handle read-only collection sub-resources. (However, it still erroneously generates a create section at the top-level, even if the sub-resource is read-only).

So, I tried adding this to the handler, to ensure that the api() method gets forwarded on to the request handler being wrapped by the ReadOnlyRequestHandler:

final class ReadOnlyRequestHandler extends AbstractRequestHandler {
    // ... lines omitted ...
    @Override
    public ApiDescription api(ApiProducer<ApiDescription> producer) {
        if (delegate instanceof Describable) {
            return ((Describable<ApiDescription, Request>)delegate).api(producer);
        }
        else {
            return super.api(producer);
        }
    }

However, this didn't work. I got this error:

java.lang.IllegalStateException: The give Resource name already exists but the Resource objects are not equal
        at org.forgerock.api.CrestApiProducer.merge(CrestApiProducer.java:129)
        at org.forgerock.api.CrestApiProducer.merge(CrestApiProducer.java:45)
        at org.forgerock.services.routing.AbstractRouter.buildApi(AbstractRouter.java:276)
        at org.forgerock.services.routing.AbstractRouter.updateApi(AbstractRouter.java:251)
        at org.forgerock.services.routing.AbstractRouter.api(AbstractRouter.java:244)
        at org.forgerock.opendj.rest2ldap.Resource.subResourcesApi(Resource.java:652)
        at org.forgerock.opendj.rest2ldap.SubResource$SubResourceHandler.api(SubResource.java:221)
        at org.forgerock.opendj.rest2ldap.SubResource$SubResourceHandler.api(SubResource.java:141)
        at org.forgerock.services.routing.AbstractRouter.buildApi(AbstractRouter.java:268)
        at org.forgerock.services.routing.AbstractRouter.updateApi(AbstractRouter.java:251)
        at org.forgerock.services.routing.AbstractRouter.api(AbstractRouter.java:244)
        at org.forgerock.opendj.rest2ldap.DescribableRequestHandler.api(DescribableRequestHandler.java:109)
        at org.forgerock.opendj.rest2ldap.DescribableRequestHandler.api(DescribableRequestHandler.java:40)
        at org.forgerock.services.routing.AbstractRouter.buildApi(AbstractRouter.java:268)
        at org.forgerock.services.routing.AbstractRouter.updateApi(AbstractRouter.java:251)
        at org.forgerock.services.routing.AbstractRouter.api(AbstractRouter.java:244)
        at org.forgerock.json.resource.FilterChain.api(FilterChain.java:272)
        at org.forgerock.json.resource.FilterChain.api(FilterChain.java:38)
        at org.forgerock.services.routing.AbstractRouter.buildApi(AbstractRouter.java:268)
        at org.forgerock.services.routing.AbstractRouter.updateApi(AbstractRouter.java:251)
        at org.forgerock.services.routing.AbstractRouter.api(AbstractRouter.java:244)
        at org.forgerock.opendj.rest2ldap.DescribableRequestHandler.api(DescribableRequestHandler.java:109)
        at org.forgerock.opendj.rest2ldap.Rest2LdapJsonConfiguratorTest.createDescribableHandler(Rest2LdapJsonConfiguratorTest.java:128)
        at org.forgerock.opendj.rest2ldap.Rest2LdapJsonConfiguratorTest.testConfigureEndpointsWithApiDescription(Rest2LdapJsonConfiguratorTest.java:67)

The issue is that the code tries to generate two different service descriptions, but use the same name for both; they both get created as frapi:opendj:rest2ldap:user:1.0. One of the descriptions is read-only, while the other is read-write. I think that the code needs to take into account whether or not the resource is read-only or writable, and incorporate that into the service name somehow so there is no conflict.

That's why the attached expected.json file does exactly that. Instead of frapi:opendj:rest2ldap:user:1.0, it becomes frapi:opendj:rest2ldap:user:read-write:1.0 and frapi:opendj:rest2ldap:user:read-only:1.0.

@GuyPaddock
Copy link
Author

Side note: given is spelled wrong. But that's likely a bug for... https://github.com/WrenSecurity/wrensec-commons ? I think?

GuyPaddock pushed a commit to GuyPaddock/wrends that referenced this issue Oct 26, 2017
Fix read-only sub-resource CREST API descr (WrenSecurity#6)
40be863 
Corrects three obscure defects in the Rest2LDAP implementation of CREST descriptors:
- Read-only sub-resources were not appearing at all in the CREST API description JSON.
- When read-only sub-resources appeared in the API description alongside writable sub-resources for the same models, the generated service name for the sub-resources was the same, leading to an IllegalStateException. Now, we generate a unique service name for each sub-resource based on its writability.
- A top-level `create` request was still being rendered in the API description for read-only sub-resources.
@GuyPaddock GuyPaddock mentioned this issue Oct 26, 2017
Merged
Kortanul added a commit that referenced this issue Oct 26, 2017
@Kortanul
Merge pull request #7 from GuyPaddock/feature/issue-6--crest-read-onl…
a3da776 
…y-desc

Fix read-only sub-resource CREST API descr (#6)
@GuyPaddock GuyPaddock changed the title Rest2LDAP CREST API descriptors ignore read-only collection sub-resources Bug: Rest2LDAP CREST API descriptors ignore read-only collection sub-resources Oct 29, 2017
@Kortanul Kortanul changed the title Bug: Rest2LDAP CREST API descriptors ignore read-only collection sub-resources Rest2LDAP CREST API descriptors ignore read-only collection sub-resources Oct 29, 2017
pavelhoral added a commit to orchitech/wrends that referenced this issue Apr 18, 2023
@pavelhoral
Revert "Fix read-only sub-resource CREST API descr (WrenSecurity#6)"
1058ece 
This reverts commit 40be863.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
🪲 bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@GuyPaddock @Kortanul