fix: Allow a client to specify the client_id in body when using Authenticate

[Kilobyte22]

Currently when a client uses Basic Auth but still specifies client_id as a body parameter, this library will raise an invalid_request error. However specifying client_id is completely acceptable to the standard, even when using Basic Auth. In fact, some client libraries, like for example the elixir oauth2 library always set the client_id parameter.

This Commit makes it so we ignore any credentials specified in the body, if Basic Auth is used

This changes/fixes/improves …

• [ ] I have read the [contribution guidelines][Contributing] - do not exist
• [ ] This change has tests (remove for doc only) - AuthCode seems to not have any test coverage, the test suite also failes to compile on current rust
• [x] This change has documentation - not needed, bugfix
• [x] Corresponds to issue (number) - none

I license this contribution under the dual MIT/Apache-2.0 license, allowing licensees to chose either at their option.

[HeroicKatora]

Including the client credentials in the request-body using the two parameters is NOT RECOMMENDED and SHOULD be limited to clients unable to directly utilize the HTTP Basic authentication scheme (or other password-based HTTP authentication schemes)

Evidently these clients are able to authenticate with the HTTP schema. Ignoring the guideline to consequently put this interface off-limits to them, the "full implications must be understood and carefully weighed before choosing a different course."

This is a security risk where different proxies might disagree over the provided authentication and dropping some might allow subversion of expectations. (The failing test is a comment to that effect). The attack surface is maybe more evident when considering that binding of redirects is quite critical to avoid some open-redirect scenarios in particular with regards to public clients. So: no, not by this motivation and test coverage, for all library consumers, at least.

[Kilobyte22]

I was not expecting this to get merged without any comment, as this was only a quick and dirty hack implicityly asking for comments. I was very much expecting to perform changes before it getting merged. I maybe should have made that clearer, I apologize for possible confusion.

I do agree that client_secret should not be permitted, however I disagree for client_id. It would probably be a good idea to ensure that, if present, it matches the one provided as part of the basic auth credentials. As the implementation stands, it is not compatible with some clients. Currently also would not even allow client_id to be set, if credentials in the body are allowed.

Interestingly the testsuite does compile in CI, it does not compile for me locally, I'll have to figure out why then.