Captain-P-Goldfish / SCIM-SDK

a scim implementation as described in RFC7643 and RFC7644
https://github.com/Captain-P-Goldfish/SCIM/wiki
BSD 3-Clause "New" or "Revised" License
121 stars 38 forks source link

Allow Content-Type=application/json for incoming requests as SCIM server #629

Closed haster closed 4 months ago

haster commented 5 months ago

This is related to #399, but with the roles reversed.

Given that not all clients can be relied upon to use the proper application/scim+json Content-Type when sending PUT or POST requests, it would be nice if we can optionally support those clients. At least (optional) support for application/json seems nice.

We can support application/json Accept-headers by simply adding

@Produces({MediaType.APPLICATION_JSON, HttpHeader.SCIM_CONTENT_TYPE})

to our rest endpoint, but the content type is explicitly validated in UriInfos.validateHttpHeaders and that method only accepts application/scim+json.

The RFC is not very clear, but seems to suggest that service providers should preferably accept application/json as well. This would also fit with the general principle of "be lenient when validating input, be strict when validating your own output".

We propose a configuration setting (I'll see if I can whip up a PR or proposal tomorrow) that would make the content-type header checking more lenient or possibly even disable it.

(Strict checking by default is fine, of course, we would just like a way to manipulate this behaviour.)

Captain-P-Goldfish commented 5 months ago

Why not handling this before calling resourceEndpoint? I have at least one case in which I have the sam eproblem. I solved it like this:

private Map<String, String> getHttpHeaders(HttpServletRequest request)
  {
    Map<String, String> httpHeaders = new HashMap<>();
    Enumeration<String> enumeration = request.getHeaderNames();
    while (enumeration != null && enumeration.hasMoreElements())
    {
      String headerName = enumeration.nextElement();
      if (HttpHeader.CONTENT_TYPE_HEADER.equalsIgnoreCase(headerName))
      {
        ...
      }
      else 
      {
        httpHeaders.put(headerName, request.getHeader(headerName));
      }
    }
    return httpHeaders;
  }

and then:

 Map<String, String> httpHeaders = getHttpHeaders(request);
    String query = request.getQueryString() == null ? "" : "?" + request.getQueryString();

    ScimAuthentication scimAuthentication = new ScimAuthentication();
    final String fullUrl = request.getRequestURL().toString() + query;

    ScimResponse scimResponse = resourceEndpoint.handleRequest(fullUrl,
                                                               HttpMethod.valueOf(request.getMethod()),
                                                               requestBody,
                                                               httpHeaders,
                                                               new Context(scimAuthentication));

Is this a suitable solution for your problem?

haster commented 5 months ago

I think the "..." part in your code snippet is the actual change you are proposing, and the rest is just boilerplate. :) If I interpret correctly:

Are you suggesting I replace an incoming Content-Type header of "application/json" with "application/scim+json" in order to satisfy the check done in UriInfos?

Captain-P-Goldfish commented 5 months ago

Actually yes. But I would do it only in case that the received header-value matches ˋapplication/jsonˋ. For content-types like xml I would'nt replace it of course :-)

haster commented 5 months ago

This would work but it feels kinda 'hinky'. Rewriting the Content-Type header to pretend it's a different one feels like the wrong way to go about this. It's not so much "accepting application/json" as "faking it to make the checks happy".

Also, there is a small chance that it could lead to problems down the line because you lose the information that it's possibly not 100% compliant with the SCIM spec (even if it can be parsed safely), which we would preserve if we kept the original header. Granted, that's somewhat hypothetical, since anyone pushing json to a scim endpoint is probably pushing scim+json (or compatible), just not saying so, but why run the risk?

Is there anything against adding a configuration option for this? I'd be ahppy to sent it in as a PR.

Captain-P-Goldfish commented 5 months ago

no, I do not have objections in general. My thought was just, that the problem is basically pertty easy to solve :-) I would suggest to log the info that the request-header is not SCIM compliant on debug-level.

haster commented 5 months ago

Thanks for the tip, and thanks for trying to give a "shortcut".

I opened https://github.com/Captain-P-Goldfish/SCIM-SDK/pull/636 for your consideration.

Reworked the UriInfos test a little to separate the various cases. I found it easier to think about this way :-)