When unsetting the default key, the confirmation text currently reads

Are you sure you want to remove the default key from KEYLABEL?

which is unclear. It should be something like

Are you sure you want to unset KEYLABEL as the default key?

Additionally, the text of the button should probably be consistent with the "unset" terminology and read "Unset Default" instead of "Remove Default".

The information about keys that appears in the table on the key listing page should be updated to be more useful.

• Remove the machine name column
• Add columns for
  • Description
  • Key provider

The two new columns should not appear at smaller screen sizes.

Additionally, the title of the page should be changed to "Keys", from "Key Configuration".

From the example I gave in #40:

[W]ith Rijndael-128, it's very important that the key be 128 bits of random data. If you were to generate a 16-character key by, say, truncating the results of a hash function like MD5, it would seem to fit that requirement. However, the keyspace would actually be reduced by a factor of 2^64. Using drupal_random_bytes(16)—or whatever the D8 replacement is—truly does produce 128 bits of random data, but it's not readable. Thus the need to base64-encode it, if you want it to be readable. But then Key needs to know to base64-decode it when retrieving it.

"Unset" operation for a default key?

Is this desirable?

The Method setting field for the File key provider is used to select a transformation that should happen to the contents of the file when the key is being retrieved. The result of the transformation is the key itself. The two available options are "file contents," which does no transformation, and "MD5 hash," which uses an MD5 hash of the file contents as the key. We should change the name of the field to better reflect its purpose—perhaps to "Transformation," "Process," "Modification"—something other than "Method," which is vague.

In addition, the File provider needs to have a new option for this field, "Base64 decode," which should base64 decode the contents of the file and use that as the key. The Drupal configuration (Simple key) provider also needs to have this field as a setting, with "Base64 decode" as an option, but I will create a separate issue for that.

Shouldn't the DocBlocks for the methods in KeyManager.php have an extra asterisk in the first line to conform with DocBlock standards?

Current:

/*
 * Loading all keys.
 *

Proposed:

/**
 * Loading all keys.
 *

In the Drupal 7 version of the Encrypt module, a plugin type called "Key Provider" was defined to allow different methods of providing a key. The name was changed to "Key Storage Method" in the Drupal 7 version of the Key module. A plugin type that defines this same functionality is needed for the Drupal 8 version of Key. "Key Type" is currently being used for that functionality, but the terminology is not correct. We could continue to use "Key Storage Method" for the label or something else—perhaps just "Storage Method," or "Location."

Currently the admin page for managing keys can be found under Structure, but the path suggests that it's under Configuration (/admin/config/system/key). We should choose one or the other and be consistent.

To make it easy for modules to integrate with Key, the version of the module for Drupal 7 adds a new form API element called key. It extends the select element, but fills #options with the list of available keys. It also provides additional instructions for using and defining keys in the #description. Here's an example of what that looks like in code:

$form['secret_key'] = array(
  '#type' => 'key',
  '#title' => t('Secret key'),
);

This functionality should be in the Drupal 8 version also. Key for D7 has additional functionality, such as filtering, that can be added later for D8.

-Add in storage method annotation field
-Add in getKeysByStorageMethod to service (filter by annotation)
-Add in getKeysByType to service (filter by plugin type)

The Drupal 7 version of Key requires a user to specifically enable integration with other modules that can use it. This gives the user more control over when Key is handling a key for another module, instead of just taking over everything it can when it's enabled. I think this is an useful feature and we should duplicate it in the Drupal 8 module. I'm open to suggestions for how to improve this functionality.

When setting or unsetting the default key, the following message is displayed:

content key: KEYLABEL is now default.

The "content key: " part of the message should not appear in either instance.

When unsetting the key, the message should say something to the effect that the key is no longer the default.

Currently key module throws fatal error in SimpleTest and on install because of Annotation changes in Drupal core: Entity annotation link templates are paths again.

I have a fix in my testing pull request soon, and will reference this issue number from the commit.

From #33:

I don't think it's possible for a contributed module to add a top-level administrative section, along with its own page, without causing complications and problems.

Problem 1: If multiple modules define the security section, multiple instances of it appear on the Configuration page.

Problem 2: If multiple modules define the route to the security section's page, only one will be used. For example, if Key and Password Policy both define the path "/admin/config/security", only one of the module's admin pages will be listed on it. In my tests, it was Key, but I suppose it depends on the loading order of the modules.

I propose that the Keys admin link be moved under the System category, unless there's an easy way to resolve the problems introduced by defining the Security category.

What is the best approach? Should Key throw an exception or return null? An exception would need to be handled with try/catch by modules that use Key so that the application does not crash, but leaves the possibility for an error message. Returning null is easier (?) for a module to handle, maybe?

Per the pull request #10 tests that discovered this.

Currently on the Key edit form, both key provider settings fields and key value fields are combined into one section. They should be split into two sections, since they are for different purposes.

Use Case

Lockr (and possibly other external key providers) store metadata about keys in their own systems, which must be accessed by some ID. Reusing the key entity's ID simplifies that process and prevents the user being required to enter a separate ID for the service in addition to one for the entity itself.

It is possible to generate a random ID to satisfy that requirement. However, if an external key provider, like Lockr, aims to allow users to list and modify their keys directly in the external service, having the key names and descriptions match makes that process much easier.

Changes

(Breaks backwards compatibility.)

I propose to change KeyProviderInterface::getKeyValue()'s function signature from:

public function getKeyValue();

to

public function getKeyValue(KeyInterface $key);

and modify the configuration form such that it allows access to the key entity values from the key provider. Currently it only passes the 'key_settings' subset of form values.

Could be as simple as changing:

$plugin_settings = (new FormState())->setValues($form_state->getValue('key_settings'));
$plugin = $this->manager->createInstance($form_state->getValue('key_provider'), []);
$plugin->submitConfigurationForm($form, $plugin_settings);

to

$plugin = $this->manager->createInstance($form_state->getValue('key_provider'), []);
$plugin->submitConfigurationForm($form, $form_state);

as well as the respective change for KeyForm::validateForm().

Another option would be to not change KeyProviderInterface::getKeyValue() and leave it up to the key provider to store relevant data from the key in KeyProviderBase::submitConfigurationForm(). Though this sounds like storing redundant information since I can't think of a reason one would change the key entity without wanting the provider to update.

If there is an alternative that makes sense and maintains backwards compatibility, I would love to hear it.

We need to be careful about differentiating between Keys and Key Providers. Throughout the module, key provider settings are referred to as "key settings". That introduces the potential for confusion and should be changed globally, using "key provider settings" instead of "key settings". Alternatively, we could use "provider settings", but I'm inclined to be more verbose for improved clarity.

If a key is set as the default and you try to make a different key the default, a fatal error is returned:

Fatal error: Cannot access protected property Drupal\key\Entity\Key::$service_default in /var/www/drupal/modules/contrib/key/src/KeyRepository.php on line 125

This error is returned after the confirmation form is submitted. If you unset the default, then set the new default, there is no error.

Of the two built-in providers, the only one that allows setting a key is Configuration. It makes sense for a key that was set previously to be displayed when the key is edited.

For security reasons, however, other providers that allow setting the key may want to indicate that the key has been set, but not actually display it. It can be up to the provider to decide how to do that. In the example I'm thinking of, it would be handled differently depending on what type of key it is: For API keys, asterisks would be displayed, except for the last four characters. For encryption keys, only asterisks would be shown.

The Drupal 7 version of Key deals with this issue by defining an "obscure key" callback function that can be defined and employed by the provider. The provider then needs to address the situation when a user submits an obscured key, recognizing that it hasn't actually been changed, so it doesn't get overwritten with asterisks (or whatever is being used as a placeholder for the actual key).

-Change the name
-Update the documentation to describe what a key provider is (e.g. explain settings, plugin, etc)

The current paths for key management look like this:

• /admin/config/system/key
• /admin/config/system/key/add
• /admin/config/system/key/{key}
• /admin/config/system/key/{key}/delete

Consider a situation where a user creates a key with a machine name of 'add'. They would never be able to edit it (though they could delete it).

We should modify the paths to match the standard Drupal pattern for paths, as follows:

• /admin/config/system/key
• /admin/config/system/key/add
• /admin/config/system/key/manage/{key}
• /admin/config/system/key/manage/{key}/delete

Password policy does this

The #empty_value is set to 'none', which is interpreted to be a key name, rather than an empty value. It should be changed to an empty string (or just not defined), unless of course we want to allow 'none' to be a valid key provider. I guess that would allow you to create a dummy key that doesn't actually return anything. Do we want that?

This can be a very useful field for users who want to add more information about a key. The Drupal 7 version of Key has it and the Drupal 8 version should, too.

When there are no keys to list on the Key Manager page, the default Drupal no-results text (“There is no Key yet.”) is displayed, which is awkward. It should be changed to something similar to what Taxonomy provides (“No vocabularies available. Add vocabulary.”).

In order to simplify retrieving a list of available keys by modules that integrate with Key, we should provide a function to the Key Repository service to retrieve a list of key names. These could then be easily used in a select form, for instance.

Here's an example of how this change would affect Encrypt:

Before:

$keys = ['default' => 'System Default'];
/** @var $key \Drupal\key\Entity\KeyInterface */
foreach ($this->key_repository->getKeys() as $key) {
  $key_id = $key->id();
  $key_title = $key->label();
  $keys[$key_id] = (string) $key_title;
}

After:

$keys = ['default' => 'System Default'];
$keys += $this->key_repository->getKeyNamesAsOptions();

Listing key names in select fields will be very common, so it's helpful to have a function that returns a list that's ready for the select FormAPI element.

Even better would be to provide a new key FormAPI element, the way Key for Drupal 7 does, but I'll create a new issue for that.

See #40.

The Drupal configuration (Simple key) provider needs a setting field for transforming the retrieved variable into a key. The default option should be no transformation, but base64 encoding should be another option, so that keys can be stored base64-encoded.

Key::get($id), return a KeyInterface
Key::default(), return a KeyInterface
Remove all references to getKeyValue in KeyRepository

Extend KeyInterface for "getKeyValue()"

Need to determine if we need this. Currently has stubbed out options (test,test2,test3, etc).

Sorry for the confusing issue title.

A standard Drupal pattern when referring to an item’s (user-friendly) label in a form is to use the term “name”. See examples below. We should change “Label” to “Key name” to be consistent with this pattern.

The default key providers available have the following names:

• Simple key
• File key

"Key" in both instances is unnecessary and "Simple key" doesn't give a clear indication of how the key is actually being provided. I propose the following two replacement names:

• Drupal configuration
• File

When a key is deleted, its provider may need to perform some action, such as deleting the actual key value, wherever it is stored. This functionality should also be triggered when the provider is changed.

Key for Drupal 7 does this with a CTools callback.

Currently, the Key Type plugin type seems to be defining what is called "Key Storage Method" in the Drupal 7 version of Key and called "Key Provider" in the Drupal 7 version of Encrypt. Key Type should actually indicate the type of key being defined. For example, a module could define a key type of "Authorize.net Transaction Key," which would allow for validation rules specific to that type of key and would allow filtering of the list of keys by modules that use them. We will need a new plugin type to add storage-method functionality. I'll create a separate issue for that.

Currently 'administer site configuration' is used for the permission to access Key administration pages. A new permission should be created that is specific to administering keys.