pingidentity / scim2

The UnboundID SCIM 2.0 SDK for Java
175 stars 72 forks source link

When returning a ScimException response with ScimExceptionMapper, id, externalId and meta should be ignored. #215

Closed MilkChocolateWisdom closed 3 months ago

MilkChocolateWisdom commented 7 months ago

Describe the bug According to 3.12. HTTP Status and Error Response Handling, the error response should be presnted like below:

{
     "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"],
     "scimType":"mutability"
     "detail":"Attribute 'id' is readOnly",
     "status": "400"
   }

This is not the case when returning getScimError() because the ErrorResponse is extending BaseScimResource.

To Reproduce Use ScimExceptionMapper to map throwing ScimException to a response. The current response is like below:

{
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:Error"
    ],
    "id": null,
    "externalId": null,
    "status": "404",
    "meta": null,
    "scimType": null,
    "detail": "Specified resource ID 728d88e-9d71-4150-92de-b5e06673edfd does not exist."
}

Expected behavior These fields should be ignored in the Error schema response:

id": null,
"externalId": null,
"status": "404",
"meta": null,

Additional context Add any other context about the problem here. For example:

kqarryzada commented 7 months ago

As you point out, this is ultimately because the ErrorResponse object (stored on a ScimException) inherits from a BaseScimResource, which is by design. So every exception message has fields like id and externalId that are inherited from the base class, but are set to null since they are not used. When null fields get printed in JSON outputs like this, it typically indicates that the ObjectMapper in use is missing a configuration setting.

Can you give some code that reproduces this issue? You mention the ScimExceptionMapper, so I tried this with a JAX-RS Exception Mapper with the following configuration, and didn't see any null fields:

    ResourceConfig config = new ResourceConfig();
    config.register(ScimExceptionMapper.class);
    config.register(new JacksonJsonProvider(JsonUtils.createObjectMapper()));

I think the key part here is the call to com.unboundid.scim2.common.utils.JsonUtils#createObjectMapper to retrieve a pre-built ObjectMapper from the SCIM SDK with the desired configuration settings. But if this isn't related to the issue, then a code snippet illustrating the problem would help.

MilkChocolateWisdom commented 7 months ago

I think I jumped the gun here. As you have mentioned, I am in fact using Jackson's Object mapping within Spring Boot and failed to provide a global setting to ignore null fields. Upon providing the below setting, I am now able to ignore id and externalId and other null fields. spring.jackson.default-property-inclusion=non_null

I am somewhat new to the ObjectMappers and I noticed that the JsonUtils's createObjectMapper you pointed out provides the necessary config settings for serializing the SCIM attributes.

In our Spring controllers, we are returning the exceptions as

@PATCH
@Path("/{tenantId}/example/example2/scim/v2/Users/{id}")
public User exampleRequest(PatchRequest patchRequest, @PathParam("id") String id, @PathParam("tenantId") String tenant) throws ScimException {

        return exampleService.patchUser(patchRequest, tenant, id);
    }

Using JerseyConfig to reegister ScimExceptionMapper: register(ScimExceptionMapper.class);

kqarryzada commented 6 months ago

That makes sense. I think it's worth grabbing an ObjectMapper from the SCIM SDK and registering it as a Spring Bean so that you don't have to set those properties via Spring. I'm pretty sure the Spring controller will then use that ObjectMapper to deserialize JSON responses to clients.