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.