Fix #349 Support for client_secret_basic only token_endpoint_auth_method

[xataxxx]

The OIDC provider I'm using only supports client_secret_basic in their token_endpoint_auth_methods_supported. This is coded in a way that it only attempts to use the header based auth when ONLY client_secret_basic is provided in discovery "token_endpoint_auth_methods_supported".

[julien-nc]

Thanks a lot for this PR. Much needed indeed.

According to https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication , client_secret_basic should even be the default when no supported auth method is listed in the discovery. token_endpoint_auth_methods_supported is described in https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata

Could you split the code into 2 sections?

• first determine which auth method should be used (default to basic auth, use body params if token_endpoint_auth_methods_supported is there and contains client_secret_post)
• then set the necessary headers or body params

Instead of always setting the body params and then unsetting them if basic auth should be used, you can set them only if basic auth is not used (remove them from the initial assignment to $requestBody).

Wdyt?

[xataxxx]

I agree with splitting the code into picking the right auth method and then applying the correct params and headers. I'll try to tidy up that part a bit by the end of the week.

I chose the current logic because I wanted to code to continue working like it is currently (and keep my code change minimal).

The current code always sends POST request. If we change the default from POST to basic if no 'token_endpoint_auth_methods_supported' is specified then it could become a breaking change for some unlucky users.

So in that regard I'm not 100% sure changing the default behaviour is the right thing to do (for Nextcloud users), even though it would be the correct thing by the spec.

If it was up to me, I would change the default to basic like the spec says and hope for the best :)

[julien-nc]

I think it's pretty safe to assume that Oidc providers which don't declare a token_endpoint_auth_methods_supported will support basic auth as it's the recommended default in the official Oidc specs.

Let's follow the official specs in user_oidc.

[xataxxx]

Changed the code, now defaulting to client_secret_basic if no supported methods is specified AND even if both basic and post is specified (as basic is default, I gave it preference).

[julien-nc]

Thanks a lot. Can you sign all the commits? There are instructions in the logs of the failing CI action. If you can't I'll force merge :grin:

[xataxxx]

I think I got it.

[github-actions[bot]]

Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

[hendrik1120]

Hi guys, this unfortunately was a breaking change as you have effectively changed the default token_endpoint_auth_method.

Just because the provider generally does support the token_endpoint_auth_method it is not guaranteed that the client is also allowed to use it. In a specific case, Authelia was configured to only allow client_secret_post for the nextcloud client, as this was the only supported auth_method at the time. Just because Authelia in this case is able to support all auth methods defined by the spec, it's not guaranteed the client can also use all of them. This is in compliance with the spec as an oidc client generally should only have one auth_method defined.

The specification is not entirely clear around this area, but I would argue that the token_endpoint_auth_methods_supported together with thetoken_endpoint_auth_method values are intended for the dynamic client registration section of the spec. As this feature is not available in this plugin, a configuration option to change the default token_endpoint_auth_method might have been the best solution instead of introducing this dynamic behavior.