Upgrade problems openid_connect 1 to 2

I've been debugging a bit in openid_connect.

It seems when moving from v1 to v2 the config data changed names, anyway my config from v1 isn't visible anymore in v2.

So I recreated my config. After that. I have my own client that derives from OpenIDConnectClientBase and that doesn't seem to be able to get its config data, e.g. the parentEntityId (which seems to be filled in in plugin initialization) is "", while the config actually has a value for it.

You probably realized that the major change from 1 to 2 is that the openid plugins are no longer part of the module configuration, but are now "config entities". That means that you can actually re-use the same plugin for different remote services, which was impossible before.

This was a major refactoring of the code, but plugins should not change that much. Please see #2899185: Use config entities for clients → .

Check https://git.drupalcode.org/project/openid_connect/-/compare/4b2c6bfe9bae... for the changes done to the Generic client until now.. Note that some of those (autodiscovery of endpoints and the end-session endpoint) are new functionalities that don't have anything to do with the changes needed to make the plugin compliant with the new functionality.

Thanks for the information... maybe an entry could be made in the readme file to explain upgrades.

I'm still not out of the woods with the problem but I pinpointed where it is.

But first, I need my own OIDC client since I need a small extension to support a "login_hint" feature. So what I do is make a new client that extends OpenIDConnectClientBase, which is empty besides a copy of the authorize() method from OpenIDConnectClientBase to which I add 2 lines. But for testing purposes I even just extended OpenIDConnectClientBase and removed all methods.

Then I get the error "Symfony\Component\Routing\Exception\InvalidParameterException: Parameter "openid_connect_client" for route "openid_connect.redirect_controller_redirect" must match "[^/]++" ("" given) to generate a corresponding URL. in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 203 of /var/www/html/www/core/lib/Drupal/Core/Routing/UrlGenerator.php)." when I go to my site.
Reason for this is because in the method below the "$this->parentEntityId" is an empty string.

  public function authorize(string $scope = 'openid email', array $additional_params = []): Response {
    $language_none = \Drupal::languageManager()
      ->getLanguage(LanguageInterface::LANGCODE_NOT_APPLICABLE);

    $redirect_uri = Url::fromRoute(
      'openid_connect.redirect_controller_redirect',
      [
        'openid_connect_client' => $this->parentEntityId,
      ],
      [
        'absolute' => TRUE,
        'language' => $language_none,
      ]
    )->toString(TRUE);

    $url_options = [
      'query' => [
        'client_id' => $this->configuration['client_id'],
        'response_type' => 'code',
        'scope' => $scope,
        'redirect_uri' => $redirect_uri->getGeneratedUrl(),
        'state' => $this->stateToken->generateToken(),
      ],
    ];

    if (!empty($additional_params)) {
      $url_options['query'] = array_merge($url_options['query'], $additional_params);
    }

    $endpoints = $this->getEndpoints();
    // Clear _GET['destination'] because we need to override it.
    $this->requestStack->getCurrentRequest()->query->remove('destination');
    $authorization_endpoint = Url::fromUri($endpoints['authorization'], $url_options)->toString(TRUE);

    $this->loggerFactory->get('openid_connect_' . $this->pluginId)->debug('Send authorize request to @url', ['@url' => $authorization_endpoint->getGeneratedUrl()]);
    $response = new TrustedRedirectResponse($authorization_endpoint->getGeneratedUrl());
    // We can't cache the response, since this will prevent the state to be
    // added to the session. The kill switch will prevent the page getting
    // cached for anonymous users when page cache is active.
    $this->pageCacheKillSwitch->trigger();

    return $response;
  }

I compared what the URLs were in openid_connect v1.2.0 and what solves it for me in v3 is this. Replacing $this->parentEntityId by $this->getPluginId() in the authorize() method.

    $redirect_uri = Url::fromRoute(
      'openid_connect.redirect_controller_redirect',
      [
        'openid_connect_client' => $this->getPluginId(),
      ],
      [
        'absolute' => TRUE,
        'language' => $language_none,
      ]
    )->toString(TRUE);

$this->getPluginID() results in my case in "acm-idm" (the system name I used for the plugin, and also the id of the OIDC server). And the URLs I get out of it are:

redirect URI = "https://mysite.be/openid-connect/acm-idm"
authorization url = "https://authentication-server.be/op/v1/auth?client_id=e243df5e-9f0d-4977...

The strange thing is also that on the client config screen the Redirect URL is calculated as "https://mysite.be/openid-connect/acm_idm"... which in my case is wrong: the acm_idm is the system name of the client config (which can't have dashes, while the actual string should "https://mysite.be/openid-connect/acm-idm" (dash instead of underscore), BUT it does find "acm_idm" while during the actual execution that part is always empty. The reason for the discrepancy is that the formbase for the config screen has its own getRedirectUrl() method which calculates the string with the system name of the client config.

So I still have 2 problems:
1. The parentEntityId doesn't get filled in properly. I see it being filled in in the initializePlugin() method of OpenIDConnectClientCollection, but I see no evidence of it actually happening.
2. If parentEntityId would be filled in properly I can't make it work since I need "acm-id" as part of the URIs, and system names in Drupal can't contain a dash, so I can't make an OIDC client system name that I can use with my OIDC server. And I'm only a user of the OIDC server, so I can't change anything to the setup there.

Some more debugging.

Number 1. got solved by running "drush updatedb", my bad.

For number 2. I would suggest to introduce a new field and use that instead of the system name of the instance of the plugin in the redirect URL. For backwards compatibility: when the new field is empty the system name could be used anyway. Sole reason is that Drupal system names can't use '-', and some OIDC providers (e.g. the one of the Belgian government) do use a dash ("acm-idm").
For the name of the name field, no idea what it should be called. Right now the redirect parameter is filled in in "parentEntityId" and it gets taken from "client_id", which it not really is in OIDC terminology.

Short term work-around for me is to override the parameter for the redirect URL in my own client, which would actually revert it a bit more like v1.

Since we only have an issue with the dash, we should better only take care of it under the hood.
I'm going to open a MR for this based on v3 of this module. This should be easily applied to v2 though I suppose too.
I also mark it as a bug report too since it's trying to fix a functionality that was working previously - and of course for other developers to also be able to find it.

Setting as ready for review after committing to https://git.drupalcode.org/project/openid_connect/-/merge_requests/84.

Addding a patch as suggested in #5. This patch will fix this error : Symfony\Component\Routing\Exception\InvalidParameterException: Parameter "openid_connect_client" for route "openid_connect.redirect_controller_redirect" must match "[^/]++" ("" given) to generate a corresponding URL. in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 203 of /var/www/html/www/core/lib/Drupal/Core/Routing/UrlGenerator.php).

I've tested #10 and I can confirm that it works properly in combination with this Keycloak related issue 🐛 Error to built redirect URL RTBC .

We're having the #10 patch in use for a while now. Works fine. Small change.

Applied the patch against the latest 2.x version.

Just to sum up, since the MR is using a param_converter approach, do we all agree that what @sboden proposed in #5 🐛 Upgrade problems openid_connect 1 to 2 Needs review is a better solution here?

So, the issue addressed is fixed by using patch in #13 🐛 Upgrade problems openid_connect 1 to 2 Needs review (while also crediting @kawaljeet-singh for #10 🐛 Upgrade problems openid_connect 1 to 2 Needs review )?

I honestly would need to test this. The plugins changed from being config objects to being config entities. So now you can have multiple Okta plugins installed each with a different name. I'm not 100% sure, but I think that the above only works as long as the machine ID of the config entity is the default value (same as plugin machine name). So if you can, please create two config entities, both with names not being the default from the same plugin and check what this function returns for those entities.

#13 is junk. I Agree 100% because the patch is malformed. Using the patch will give you errors. Updated the code which should work now.