Update concern regarding UUID in Remote config entity and switch to key value store

Because the UUID generated will be different per environment, when using a config import, the UUID will then differ from the one used to create the state.

Perhaps I'm missing a link in the logic chain, but I don't think this is correct. Here's how I see it.

A Remote is created and the authorization plugin is chosen. The configuration for this plugin, including its UUID is included as a child of the remote's config:

uuid: ce866f4e-ba8b-4e2c-9038-800e2ac1fb23
langcode: en
status: true
dependencies: {  }
id: test_remote
label: 'Test Remote'
url: 'http://example.com'
auth:
  pid: oauth
  uuid: 33f847c6-4024-4d7b-b5d8-df95a398c4dc
  verified: false
  data:
    credential_provider: entity_share
    storage_key: 33f847c6-4024-4d7b-b5d8-df95a398c4dc

When \Drupal\entity_share_client\Entity\Remote::getAuthPlugin is run the plugin configuration is loaded:

/**
   * {@inheritdoc}
   */
  public function getAuthPlugin() {
    $pluginData = $this->auth;
    if (!empty($pluginData['pid'])) {
      // DI not available in entities:
      // https://www.drupal.org/project/drupal/issues/2142515.
      /** @var \Drupal\entity_share_client\Plugin\ClientAuthorizationManager $manager */
      $manager = \Drupal::service('plugin.manager.entity_share_auth');
      $pluginId = $pluginData['pid'];
      unset($pluginData['pid']);
      return $manager->createInstance($pluginId, $pluginData);
    }
    return NULL;
  }

The plugin id is extracted from this array so that the remaining keys form a configuration array, and the plugin is loaded with this config. Once the plugin has been created the UUID is persistent.

What varies by environment is the value stored in State. But in my view it should vary, especially for Oauth. Each environment should be using it's own consumer on the hub. I'm fairly certain that a variety of issues that I saw in testing were the result of environment B getting a new token from the same consumer used by environment A, causing the requests from A to fail because they were sharing a consumer and the token stored by A had been invalidated by the new token creation for B.

Thanks for clarifying. Now I understand. I do like the idea of a dedicated key-value collection. That's a great idea that accomplishes the intent of my use of State but with all the benefits that you name.

I'll have a close look at the update hook to see if I have a suggestion for this case.

See what you think of this approach?

As I read your approach, it is to always use the same uuid in the update, so if there is a config import following, and that config is from an update that took place in a prior environment, it overwrites the uuid with the same value. Result: only one value is stored.

My code was aiming to solve the problem a different way, but your way is simpler. I like it.

Will the key/value change be in this issue or another?

I can see why you want to use a common value for the auth plugin uuid in RemoteForm when a plugin has been previously stored. I had the get past the idea that in one sense it was no longer "universally unique." But we are really using it as a storage key.

        // Ensure all plugins will have the same uuid in the configuration to
        // avoid duplication of entries in the key value store.
        if (!is_null($this->authPlugin)) {
          $existing_plugin_configuration = $this->authPlugin->getConfiguration();
          $plugin_configuration = $plugin->getConfiguration();
          $plugin_configuration['uuid'] = $existing_plugin_configuration['uuid'];
          $plugin->setConfiguration($plugin_configuration);
        }

However, I think this is better handled by `ClientAuthorizationPluginManager::getAvailablePlugins`.

In ClientAuthorization we are inconsistent with the value of the key used for storing into KeyValue, and I see some other inconsistencies about when values are set and deleted. For example, in some places `-oauth` is appended to the end of the uuid to make a key and in some not. Some of that may trace back to my original draft, or the blending of our code. I'm auditing all the places that we store or retrieve and refactoring it to be consistent.

I have a proposed patch in progress but I'm being pulled back into client work that's needed first and then I'll circle back

With fresh eyes the "inconsistency" that I thought I saw was in my memory and not the code. I added some comments instead. But I did move the common uuid process as discussed above. Here's a patch against the latest 3.x with an interdiff as well.

Almost, or we could be done depending on what you think. Good catch on Oauth having secondary storage. That prompts me to say that perhaps we should establish a convention of using the plugin id and make this more general. I've refactored one more time with that in mind.