PKCE Parameter Support

This works for us. Though our custom plugin needs to unset the client secret and disable the checkbox on the form because for security reasons PKCE is required and sending the secret is prohibited.

But this is easily achieved with:

  /**
   * {@inheritdoc}
   */
  protected function getTokenRequestOptions(string $authorization_code, string $redirect_uri): array {
    $options = parent::getTokenRequestOptions($authorization_code, $redirect_uri);
    unset($options['form_params']['client_secret']);

    return $options;
  }

So this patch works for us, and since it is opt in, I can RTBC it.

I'm a little confused about how to make the PKCE work with the OAuth2 since client_secret is required. Should I create a custom plugin?

Attaching patch from MR#59 to use with composer.

I don't really like the explosion of settings in the settings form. Can we move the "On/Off" switch of this into the transformation settings, please? So the transformation options would be "Off / SHA-256 / Plain".

Also this needs a hook_update to add this setting to existing installs.

In the meantime I can confirm this is working on a D10/PHP8 site with PKCE/SHA256 enabled.
For me the use of the setting in the form is very clear (off, on - then choose transformation) but fine if it would be 1 toggle. That also prevents having 'no' transformation on existing installs when enabling the config only via code - without saving the settings form.

I rerolled the 2.x branch into 3266205-pkce-flow-2022-10--3.x.

I am using this version and everything works as expected here.
The update hook has been added to set the default config of existing plugins.

Note that the new session service that gets injected into the base client will require a change in other contrib modules that provide a client (like openid_connect_windows_aad).

interX → changed the visibility of the branch revert-ec71b33d to hidden.

I can't apply patch from MR 96 to 3.0.0-alpha2 with composer. Anybody has a working patch for 3. or 2. ?

You have to use the dev-version to test the patch. composer.json should have an entry like:

"drupal/openid_connect": "^3.x-dev"

Tested.. again.. PKCE is a widely used standard and this issue is now open for more than 2 years keeping us busy with MR's and patches that were tested and reviewed... I do not think anyone is offended by an extra setting in the mentioned config form so please make another alpha release - it is alpha for a reason and setting can easily be skipped if not needed.

Can this pkce patch be released in next alpha release?

jcnventura → closed merge request !96

jcnventura → closed merge request !59

Several problems still stand.

1. Nobody did anything regarding my comment on #23
2. The MRs are broken at the moment. Among others they reintroduce file_save_data() which was removed in Drupal 10. Merging this MR would remove support for Drupal 10. I'm closing the MRs, as I believe it might be best to start from the patch in #22
3. Not sure what the RTBCs are for, if for the #22 patch or the MRs. Hopefully it's for the patch. It looks good. As long as it is simplified somewhat to have a single toggle as I requested in #23.

This only needs someone to improve on #22 (maybe open a new MR with the contents of that patch as first commit, and then do the changes I requested in #23 as a second commit). And then someone else marks it as RTBC and it should get merged fast.

I have recreated a MR from 3.x with remarks from #35. The property name pkce_challenge_transformation was kept, so anyone using one of the previous patches can update without issue. The property use_pkce can be removed from config.

Nice. Hopefully someone can also test and review it.

Nice. Hopefully someone can also test and review it.

We have tested the patch with "S256" and it works. I couldn't test it with "plain" as our SSO provider doesn't support it.

The patch breaks login gov integration. Why do we have to rename functions defined in asbstract classes? This can break other classes extending base classes.

Renaming getUrlOptions to getAuthorizeRequestOptions and getRequestOptions to getTokenRequestOptions is breaking functionality.

Thanks for the work on this MR, I am looking for PKCE support.

Agreed with comment #40: why is the abstract class changed? There is a bunch of implementations for OpenID Connect clients that implement this class, e.g. the ever so popular openid_connect_windows_aad module (see https://git.drupalcode.org/project/openid_connect_windows_aad/-/blob/24e...). These changes will break all those implementations.

Please don't use the patch. MR113 is the current state of the work on this. And it is still under "Needs review" status.

Right, thanks for that, I completely read over that comment earlier.

The MR applies fine, and testing it with a Drupal-to-Drupal setup (using simple_oauth on the IdP side), this seems to work perfectly.

Two things that I encounter:
1. The client secret is still required to be filled in. Any text will work, since it is not actually used.
2. The MR does break other clients implementing OpenIDConnectClientBase, e.g. openid_connect_windows_aad:

```
ArgumentCountError: Too few arguments to function Drupal\openid_connect\Plugin\OpenIDConnectClientBase::__construct(), 11 passed in /var/www/html/docroot/modules/contrib/openid_connect_windows_aad/src/Plugin/OpenIDConnectClient/WindowsAad.php on line 65 and exactly 12 expected in Drupal\openid_connect\Plugin\OpenIDConnectClientBase->__construct() (line 143 of modules/contrib/openid_connect/src/Plugin/OpenIDConnectClientBase.php).
```