HTTP PATCH request made to resource server does not have a body

[MFAshby]

Describe the bug
When making an HTTP PATCH request using the resource() method of the Client object, the request params are transmitted as query parameters instead of being included in the request body. MSDN guidelines indicate that POST, PATCH and PUT should all have a request body. This is causing my request to fail, since the resource server in question is expecting a request body.

One can see that in the resource() method, only a POST request will have a body, and for the other verbs the params are transmitted as query parameters, which I believe is incorrect.
https://github.com/panva/node-openid-client/blob/0ab81fa0fae20533996953dd1a278fe6ebcc93ef/lib/client.js#L1020

To Reproduce
I'll try to publish a minimal reproduction of the issue shortly

Issuer and Client configuration: (inline or gist) - Don't forget to redact your secrets.

// Issuer configuration (issuer.metadata) and how it is constructed (discovery or manual?)
{
  // ...
}
// Client configuration (client.metadata) and how it is constructed (fromUri or manual?)
{
  // ...
}

Steps to reproduce the behaviour:

Expected behaviour
HTTP PATCH and PUT requests made to a resource server using the resource() method should have a request body.

Environment:

• openid-client version: 3.11.0
• node version: v12.14.0

Additional context

• the bug is happening on latest openid-client too.
• i have searched the issues tracker on github for similar issues and couldn't find anything related.

[panva]

I agree it should be handled, but i'd like to point out that as-is it's not declared as supported, both docs and types declare only GET and POST are supported.

I'm fine with adding more supported http methods but that said we're also only supporting x-www-form-urlencoded bodies and that'll likely have to be extended too.

[MFAshby]

Ah thanks for the response. I hadn't noticed that only GET and POST were defined in the types https://github.com/panva/node-openid-client/blob/0ab81fa0fae20533996953dd1a278fe6ebcc93ef/types/index.d.ts#L416

I think it would be valuable to support the full range of HTTP verbs, since some REST APIs make use of all of them.

Regarding the content encoding, I seem to be able to send a JSON payload, and set the Content-type header no problem. I am sending the authorization in the header not the body though, so I guess it won't work if I had to send the access_token as part of the body.

[panva]

Please see d713512 and come back with feedback. Thank you.

[idasbiste]

Hello, @panva
I've been working with @MFAshby in the same project. After applying the patch from your commit we have a problem related to one of the project dependencies - got.
The problems occurs in the specific case of a 204 No Content response with its body empty and if the json option is set to true. In that case we hit https://github.com/sindresorhus/got/blob/v9.6.0/source/as-promise.js#L60. Debugging it, I've checked that response.body is a truthy empty Buffer and when JSON-parsing it in the following instruction, it just throws a ParseError: Unexpected end of JSON input.
Upgrading the dependency to the latest version should solve the problem but we hit something else when doing Issuer.discover because openid-client is passing json: true in a GET method and got won't like it.
Any suggestions here?
Thanks!

[panva]

The problems occurs in the specific case of a 204 No Content response with its body empty and if the json option is set to true. In that case we hit sindresorhus/got:source/as-promise.js@v9.6.0#L60 . Debugging it, I've checked that response.body is a truthy empty Buffer and when JSON-parsing it in the following instruction, it just throws a ParseError: Unexpected end of JSON input.

Yeah, you should be able to pass json: false, form: false and body as string or Buffer which is already the right serialization.

Upgrading the dependency to the latest version should solve the problem but we hit something else when doing Issuer.discover because openid-client is passing json: true in a GET method and got won't like it.

This is don't follow at all. ^9.6.0 is the only got version openid-client depends on, you can't upgrade that because there weren't any more 9.x updates and 10.x would be a breaking dependency update.

Bottom line, tho, i don't want to deal with any of this body / response encoding, so i'll be removing the form and json options and instead expect body to be a string or a buffer. The developer will take care of serializing his object to a body payload.

[MFAshby]

Bottom line, tho, i don't want to deal with any of this body / response encoding, so i'll be removing the form and json options and instead expect body to be a string or a buffer. The developer will take care of serializing his object to a body payload.

This seems reasonable, the only value added when the openid-client library manipulates the body is adding the access token in the correct parameter. I don't think it's worth it for the added complexity.

[MFAshby]

This is don't follow at all. ^9.6.0 is the only got version openid-client depends on, you can't upgrade that because there weren't any more 9.x updates and 10.x would be a breaking dependency update.

We were hoping that it wasn't a very breaking upgrade, but it was.

[panva]

I took a stab at this in b4ec0d8, i have to deprecate the resource method because it was just too messy to fix it.

One can now pass headers as an object via options, query as part of the first argument which can now be a URL instance, body as a buffer or string via options. Return got response body is now consistently a Buffer.

[MFAshby]

Thanks @panva , I hope to take a look at this later in the week.