PKCE Parameter Support

A branch 3266205-pkce-parameter-support-backport-1.2 has been added to backport the support of PKCE in the 8.x-1.2 version.
I left it in draft because I'm not sure that it is the main target here (it has been added because I need this version to support PKCE and maybe others may need it).

Hi there. Apologies if the MR / new branch is unnecessary; I'm not sure what the etiquette is for continuing work on someone else's branch but there have also been some significant changes in the 2.x branch since the original MR from Dylan Donkersgoed. So I elected to make a new branch and MR.

The latest MR is based on the original, targeted at latest 2.x changes. It has some differences from the original:

- Adds configuration settings to the base form so that clients can selectively enable PKCE flow. Adding it to the base client config does mean that updating config and exporting can mean that existing clients can export some unused settings, but it seems better to keep it on the base config rather than replicating the change across clients and their schema.
- Allow selecting between S256 and plain code_challenge transformations per the RFC spec: https://www.rfc-editor.org/rfc/rfc7636#section-4.2
- Some changes in how the code_verifier value is generated--using random_bytes for cryptographically suitable randomness.
- Uses the new session service to store the code_verifier value.

I've tested this against an Okta app with PKCE required and confirmed functioning. Have not tested this with a Google client or other service yet.

Usually it would be better to continue from the one existing MR. I've closed the others to make sure that happens now.

But the tests must pass before anything happens here,

Thanks for the clarification. Regarding tests, I'm not sure how they are run on the d.o infra, but the test fails look to be from missing interface from the externalauth module dependency. Is that module added during the test run so that the interface is available?

1) Drupal\Tests\openid_connect\Unit\Entity\OpenIDConnectClientEntityTest::testGetPluginId
PHPUnit\Framework\MockObject\UnknownTypeException: Class or interface "Drupal\externalauth\AuthmapInterface" does not exist

/var/www/html/vendor/phpunit/phpunit/src/Framework/MockObject/Generator.php:167
/var/www/html/vendor/phpunit/phpunit/src/Framework/MockObject/MockBuilder.php:140
/var/www/html/modules/contrib/openid_connect/tests/src/Unit/Entity/OpenIDConnectClientEntityTest.php:94
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:673
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:673
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

I can run the test locally and it works, but that's because the module is present and installed. It also looks like other MRs have the same fails, e.g. https://www.drupal.org/project/openid_connect/issues/3281137

----

EDIT: Fixed with adding the externalauth dependency to the module composer.json

That change is not needed, nor desired. Just want to check if reverting it fixes it, as something is weird.

The externalauth module should be included by default, without having to duplicate the dependency information that is already in the .info.yml.

OK. The test failed.. That is disappointing. I guess something is cached wrongly somewhere, and the dependencies are not correctly obtained from d.o.

Anyway, needs a few more reviewers to confirm the RTBC status.

Thanks for the check and update. FWIW, I think the composer.json dependency listing is required. Mainly because the .info.yml only specifies a what (externalauth) but nothing else beyond that, e.g. what version of externalauth.

EDIT:
Per https://www.drupal.org/docs/creating-custom-modules/add-a-composerjson-file

A contributed module that depends on other modules/packages and has automated tests that run on the DrupalCI environment must have a composer.json that expresses the dependencies. Tests of merge requests on the module will fail without a composer.json file, as will tests of patches that change the dependencies.

Yes, but also: "If a module does not have any dependencies, or the dependencies are solely other Drupal modules, then a composer.json is not required. However, having a composer.json does not have a negative impact either."

This should work. There's something wrong in this issue, however...

See #3292527: Login route to a client that works just fine.

EDIT: I see now.. It only affects tests of merge requests... Well, that's probably the first advantage for using patches that I've seen in some time..

OK. Tests pass again, since I've committed already the composer.json changes. Learned something with these last comments. Thanks!

Cool, thanks for making the composer.json changes upstream.

I've merged in origin/2.x changes and the MR should be mergeable again. Not sure what the policy/etiquette is on rebasing and force pushing up, so it's just a merge commit that accounts for the latest 2.x changes.

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).

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.