Always load clients through the ClientRepository service

This patch implements the bare minimum functionality by loading the ClientRepository via the grantManager property that is accessible to all of the OAuth Controllers. This required making the $clientRepository a public property.

This patch implements the same functionality but by loading the ClientRepository via the Drupal::service('simple_oauth.repositories.client') method within each OAuth controller. It seems like this may be less efficient than going through the grantManager class, but doesn't require making the $clientRepository apublic property.

This patch builds upon the 3167287-3.patch to make the consumer client_id field name configurable via the simple_oauth.settings.consumer_client_id_field_name config value (and defaults to uuid).

This was easier to implement than I thought it would be! Having this available in config is much easier than overriding and extending the simple_oauth.repositories.client service to change the client_id that is used.

Note that the Drupal 9.1 tests that are failing are due to this issue: https://www.drupal.org/project/simple_oauth/issues/3167509

Looking into it further I realized that the validateClient() method that is documented for league/oauth2-server was only introduced in the most recent version of the library. There is another issue open to consider upgrading the library to 8.0. The main catch is that there are some breaking changes in the library.

Overall the upgrade seems quite sensible, but shouldn't be considered as blocking this feature request.

I've re-rolled this patch to work with the recent 5.x release. The only change this patch makes is ensuring that OAuth Clients are always loaded via the simple_oauth.repositories.client. service. I've injected the service as a dependency in the required controllers; I believe I found everywhere this is needed?

This is the bare minimum required to implement the client_id features discussed above. Without this, code must be changed in multiple places to in order to modify behavior that I believe the ClientRepository service should be solely reliable for. Updating the issue title to better reflect these changes.

Interesting. So in farmOS we are overriding the ClientRepository service so that we can modify ClientRepository::getClientEntity() function to load entities by a string instead of UUID. It seems this error happened when testing an incorrect client_id: https://github.com/farmOS/farmOS/blob/059e0af8d978b6dca646be3e1a4e252c75...

One of the latest commits to simple_oauth introduced a public function getClientDrupalEntity(string $client_identifier) helper function that we will probably want to use within farmOS: https://git.drupalcode.org/project/simple_oauth/-/commit/7ed0076c7bd5485...

But I think a bug was introduced with this new helper function. The ClientRepositoryInterfeace::getClientEntity() function should return ClientEntityInterface|null, but the latest change has it *always* returning a ClientEntityInterface, which will error when getClientDrupalEntity() returns NULL. The current annotation for this additional function is incorrect as well.

At the very least I think an empty($client_drupal_entities) check needs to be moved and/or added to the getClientEntity() function.

  /**
   * {@inheritdoc}
   */
  public function getClientEntity($client_identifier) {
    return new ClientEntity($this->getClientDrupalEntity($client_identifier));
  }

  /**
   * Get the client Drupal entity.
   *
   * @param string $client_identifier
   *   Client ID.
   *
   * @return \Drupal\consumers\Entity\Consumer
   *   The loaded drupal consumer (client) entity.
   */
  public function getClientDrupalEntity(string $client_identifier) {
    $client_drupal_entities = $this->entityTypeManager
      ->getStorage('consumer')
      ->loadByProperties(['uuid' => $client_identifier]);

    // Check if the client is registered.
    if (empty($client_drupal_entities)) {
      return NULL;
    }
    return reset($client_drupal_entities);
  }

  /**
   * @{inheritdoc}
   */
  public function validateClient($client_identifier, $client_secret, $grant_type) {
    if (!$client_drupal_entity = $this->getClientDrupalEntity($client_identifier)) {
      return FALSE;
    }
    $secret_field = $client_drupal_entity->get('secret');

    // Determine whether the client is public. Note that if a client secret is
    // provided it should be validated, even if the client is non-confidential.
    // The client_credentials grant is specifically special-cased.
    // @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.4
    if (!$client_drupal_entity->get('confidential')->value &&
      $secret_field->isEmpty() &&
      empty($client_secret) &&
      $grant_type !== 'client_credentials') {
      return TRUE;
    }

    // Check if a secret has been provided for this client and validate it.
    // @see https://datatracker.ietf.org/doc/html/rfc6749#section-3.2.1
    return (!$secret_field->isEmpty())
      // The client secret may be NULL; it fails validation without checking.
      ? $client_secret && $this->passwordChecker->check($client_secret, $client_drupal_entity->get('secret')->value)
      : TRUE;
  }

Okay, I was confused why this wasn't being caught by the simple_oauth tests but now it makes sense. Because the ClientRepository service is *not always used to load clients*, there are numerous places where other safety checks are performed. As soon as we move forward with this issue it makes sense that we need to move these safety checks into the ClientRepository service. But I think this is good and further demonstrates the motivation behind this issue!

But I think a bug was introduced with this new helper function. The ClientRepositoryInterfeace::getClientEntity() function should return ClientEntityInterface|null, but the latest change has it *always* returning a ClientEntityInterface, which will error when getClientDrupalEntity() returns NULL. The current annotation for this additional function is incorrect as well.

So... considering this, and looking closer at #10 again, I'd like to propose we use that approach for the ClientRepository::validateClient() *instead* of adding the getClientDrupalEntity() helper function altogether.

I think we've forgotten about the ClientEntityInterface::getDrupalEntity() function that can be used instead.

Adding a patch with this approach.

Okay, here's to revitalizing this issue. There's a lot of info above: we added new patches in #10 #12 and #15 to keep up with changes to simple_oauth 5.x. Comments above are largely just figuring out what those changes were and how it affected our patches. The original issue description is still quite accurate. But just to reset and highlight some of the motivations for this issue:

From #10 (true of this MR as well)

This is the bare minimum required to implement the client_id features discussed above. Without this, code must be changed in multiple places to in order to modify behavior that I believe the ClientRepository service should be solely reliable for.

From issue description:

This would work if the ClientRepository->getClientEntity() method was used consistently. Unfortunately the consumer entities are hard coded to be loaded by the UUID in the simple_oauth Oauth2Token, Oauth2AuthorizeController and Oauth2AuthorizeForm controllers. Since each of these classes has an instance of the Oauth2GrantManager (which has a reference to the ClientRepository class), it should be possible to always use the ClientReposiory->getClientEntity() method for loading consumer entities.

Thus; the proposed solution needs to do two things:
1) Use dependency injection to ensure the ClientRepository simple_oauth.repositories.client service is consistently used in all of the controller, form and manager classes. (this is the bulk of the changes in the diff)
2) Ensure ClientRepository::getClientEntity() is working correctly (essential - it should be the single method used to get clients). As identified in #14 I believe the new ClientRepository::getClientDrupalEntity() function introduces a bug in the event the Drupal entity is not loaded. Instead of this new "helper" method, we should just use the existing ClientEntityInterface::getDrupalEntity() function to get the Drupal entity. See patch for how this is resolved, it is quite a simple change.

I've opened a new merge request with the patch from #15 rebased onto the latest 5.2.x branch. @bradjones1 if you could please advise on how to run tests for this MR? In the past I was able to run the simple_oauth tests on my local but now they are failing (even without any of these modifications to 5.2.x). These failures seem to be the same failures that we are getting when running the farmOS API tests with patch #15 applied so there must be something related.

Test result (running phpunit in our docker container - yes this might not be the best way to do this, but used to work? For reference, here are the related farmOS test failures: https://www.drupal.org/project/farm/issues/3282186#comment-14536882)

$ phpunit /opt/drupal/web/modules/simple_oauth
PHPUnit 9.5.20 #StandWithUkraine

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing /opt/drupal/web/modules/simple_oauth
................F....................EEE.........                 49 / 49 (100%)

Time: 04:25.714, Memory: 20.00 MB

There were 3 errors:

1) Drupal\Tests\simple_oauth\Functional\RolesNegotiationFunctionalTest::testRequestWithRoleRemovedFromUser
Trying to access array offset on value of type null

/opt/drupal/web/modules/simple_oauth/tests/src/Functional/RolesNegotiationFunctionalTest.php:160
/opt/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

2) Drupal\Tests\simple_oauth\Functional\RolesNegotiationFunctionalTest::testRequestWithRoleRemovedFromClient
Trying to access array offset on value of type null

/opt/drupal/web/modules/simple_oauth/tests/src/Functional/RolesNegotiationFunctionalTest.php:220
/opt/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

3) Drupal\Tests\simple_oauth\Functional\RolesNegotiationFunctionalTest::testRequestWithMissingScope
Trying to access array offset on value of type null

/opt/drupal/web/modules/simple_oauth/tests/src/Functional/RolesNegotiationFunctionalTest.php:276
/opt/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

--

There was 1 failure:

1) Drupal\Tests\simple_oauth\Functional\PasswordFunctionalTest::testPasswordGrant
Failed asserting that 500 matches expected 200.

/opt/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/opt/drupal/web/modules/simple_oauth/tests/src/Functional/TokenBearerFunctionalTestBase.php:167
/opt/drupal/web/modules/simple_oauth/tests/src/Functional/PasswordFunctionalTest.php:81
/opt/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

ERRORS!
Tests: 49, Assertions: 366, Errors: 3, Failures: 1.

Sorry @bojan_dev I wasn't able to review your merge request before it got merged. I'm glad we're switching to using the client_id on the consumer entity as that was the original motivation for this issue. However, I originally opened this issue to address a more generic issue that we should always be using the client repository to load the consumer entity. It wasn't clear from your comment in #27 if this was the approach you would take.

With your change there still are multiple places where the logic is hard-coded to load consumers by their client_id. It's my understanding the client repository service exists as an abstraction to consolidate this logic into a single place (please correct me if this has changed in 6.x). I agree with #16 that consistently using this service will make this more secure and maintainable going forward.

Marking as "Needs work" as I don't think the original issue is "Fixed". IMO we need to update the issue/description as to what was actually changed, mark "Closed (won't fix)", or agree to implement a solution to the original issue.

@bojan_dev I'm happy to provide a MR for this, just want your opinion before I start anything. It would be a great opportunity for myself to look closer at the 6.x code and implementation which I just have not had the time to do.

Thanks @bojan_dev this LGTM! I'm not able to test too extensively at the moment locally but did include this patch in our farmOS automated tests and it worked.