Make non-confidential clients configurable for PKCE

Thanks for the patch! I left some considerations, please let me know what you think.

1. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -120,22 +139,13 @@ class Oauth2AuthorizeController extends ControllerBase {
   -      watchdog_exception('simple_oauth_extras', $exception);
   -      return RedirectResponse::create(Url::fromRoute('<front>')->toString());
   

   Why are we losing this?

2. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -120,22 +139,13 @@ class Oauth2AuthorizeController extends ControllerBase {
   -    $client_drupal_entities = $consumer_storage
   -      ->loadByProperties([
   -        'uuid' => $client_uuid,
   -      ]);
   

   Is there any other benefit to introduce the entity storage service? We can already do this with the currently injected dependencies. If there is no additional benefit, I'd rather keep the original code.

3. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -168,7 +178,7 @@ class Oauth2AuthorizeController extends ControllerBase {
   -        $server = $this->grantManager->getAuthorizationServer($grant_type);
   +        $server = $this->grantManager->getAuthorizationServer($grant_type, $client_drupal_entity);
   

   As I understand, this is the main goal of the change in this file. Would your patch work if we only had this change in this file?

4. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeForm.php
   @@ -133,9 +151,18 @@ class Oauth2AuthorizeForm extends FormBase {
   +      $client_drupal_entity = $this->entityRepository
   +        ->loadEntityByUuid('consumer', $client_uuid);
   

   Let's use the entity manager to get the storage to load entities by UUID. This way we can avoid the additional dependency on the entity.repository service.

+++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
@@ -119,13 +118,7 @@ class Oauth2AuthorizeController extends ControllerBase {
-    try {
-      $consumer_storage = $this->entityTypeManager()->getStorage('consumer');
-    }
-    catch (InvalidPluginDefinitionException $exception) {
-      watchdog_exception('simple_oauth_extras', $exception);
-      return RedirectResponse::create(Url::fromRoute('<front>')->toString());
-    }
+    $consumer_storage = $this->entityTypeManager()->getStorage('consumer');

Why did you drop the error handling here? This was useful for static code analysis and in case some nasty overwrite was in place.

+++ b/src/Plugin/Oauth2GrantManager.php
@@ -142,9 +144,14 @@ class Oauth2GrantManager extends DefaultPluginManager implements Oauth2GrantMana
+    if ($client && ($grant instanceof AuthCodeGrant) && ($client->hasField('pkce') && $client->get('pkce')->first()->value)) {
+      $grant->enableCodeExchangeProof();
+    }

Let's extract some booleans before hand so we can make the if statement more readable.

if ($grant_is_auth_code && $client_has_pkce_enabled) {
  // …
}

+++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeForm.php
@@ -133,9 +133,19 @@ class Oauth2AuthorizeForm extends FormBase {
+    $consumer_storage = $this->entityTypeManager->getStorage('consumer');

We probably want that awkward error handling here as well.

This patch is pretty impressive! Thanks for the work. As far as I can tell we are only missing 2 things before merging:

1. Testing for the "new grant".
2. An update hook to install the new base field to existing sites. The simple_oauth.install file should have examples of this.

Again, I'm so grateful to get patches with such great quality work.

Great job! Current tests are green now.

@bradjones1 is there any chance I can motivate you to contribute the tests for last leg of this issue?

I recently merged Simple OAuth Extras into Simple OAuth. This means that the patch no longer applies. However, this will make things easier in the future.

Taking the tag off as the patch was rerolled.

Or maybe not, as the patch uploaded failed to apply ;)

Rerolled #14. Interdiff isn't much of a help, cause of the simple_oauth_extras merge. It's a straight reroll plus the hook_update for updating the consumer entity (adding the pkce basefield). Lets see what the testbot has to say.

The changed catch clause in Oauth2GrantManager::getAuthorizationServer caused the failures. Lets try this one.

Okay, seems somebody can code the tests now :)

- Adding tests
- Addressing #11

ready for review

This is a great patch! Thanks for all the effort you put into this.

Thanks for contributing @bradjones1, @olafkarsten, @rpayanm, and @AjitS! This module is better and more useful thanks to you. Open source maintainers need contributions to keep up. ❤️

I granted some additional commit credit because this was tricky!