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.

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

Refuse APPLICATION_JSON to avoid data loss #390

Closed
wants to merge 1 commit into from
Closed

Refuse APPLICATION_JSON to avoid data loss #390

wants to merge 1 commit into from

Conversation

symphony-enrico
Copy link
Contributor

If APPLICATION_JSON is accepted when posting a SCIM entity, the custom JacksonXmlBindJsonProvider is not used. This is a serious problem, because the REST API appears to work without any errors, but in reality all data relating to the use of a schema extension is lost. This behavior can confuse the user and be very dangerous, it is better to reject the application/json format

If APPLICATION_JSON is accepted when posting a SCIM entity, the custom JacksonXmlBindJsonProvider is not used. This is a serious problem, because the REST API appears to work without any errors, but in reality all data relating to the use of a schema extension is lost. This behavior can confuse the user and be very dangerous, it is better to reject the application/json format
@symphony-enrico
Copy link
Contributor Author

@BacemSymphony

@bdemers
Copy link
Member

bdemers commented Oct 11, 2023

Do you want to try tweaking the JSON provider?

https://github.com/apache/directory-scimple/blob/develop/scim-server/src/main/java/org/apache/directory/scim/server/rest/ScimJacksonXmlBindJsonProvider.java#L37

My preference would be to try to make sure everything works with application/json. But if that doesn't work we could merged this for the short term. (as you mentioned, it's better to NOT loose data)

Per spec:

Service providers MUST support the "Accept" header "Accept: application/scim+json" and SHOULD support the header "Accept: application/json", both of which specify JSON documents conforming to [RFC7159].

@symphony-enrico
Copy link
Contributor Author

Do you want to try tweaking the JSON provider?

https://github.com/apache/directory-scimple/blob/develop/scim-server/src/main/java/org/apache/directory/scim/server/rest/ScimJacksonXmlBindJsonProvider.java#L37

My preference would be to try to make sure everything works with application/json. But if that doesn't work we could merged this for the short term. (as you mentioned, it's better to NOT loose data)

Per spec:

Service providers MUST support the "Accept" header "Accept: application/scim+json" and SHOULD support the header "Accept: application/json", both of which specify JSON documents conforming to [RFC7159].

My concern is that if the framework is integrated in a project that implements other endpoints (not related to scim protocol), using scim custom mapper for all endpoints (standard ones in application/json) it could be a problem.

I don't know if it is possible to bind the custom mapper only for scim related endpoints... or at least to have a configuration option to bind custom object mapper to json or not according needs... WDYT ?

@bdemers
Copy link
Member

bdemers commented Oct 11, 2023

I think that is a valid point, but you should be able to define multiple JAX-RS apps in a project.

bdemers added a commit that referenced this pull request Nov 3, 2023
Previously application/json requests may have lost any custom extension data.

This change adds applicaiton/json to the list of supproted media types (as recommended by the SCIM specs), but restricts it's types to ONLY classes known by SCIMple, and allows other MessageBodyReader/Writer to handle other requests

Fixes: #390
@bdemers bdemers closed this in #404 Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants