Key will include a generic Encryption key type that can be extended by other plugins, such as AES Encryption. However, the Encrypt module should define that key type, so it should be removed from Key.

CommentFileSizeAuthor
#3 remove_aes_encryption-2664912-3.patch2.65 KBrlhawk

Comments

rlhawk created an issue. See original summary.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented

Some of the functionality currently in AES Encryption should be moved into the generic Encryption key type. I'm thinking these ones:

  • buildConfigurationForm: This should be a form with one field for key size.
  • validateConfigurationForm: This is empty, because validation of the form is optional.
  • submitConfigurationForm: Default behavior, take the form values and using them to set the plugin configuration.
  • generateKeyValue: This looks for a field named 'key_size' in the configuration and, if it exists, uses its value to generate a random key of the specified length.
  • validateKeyValue: This looks for a field named 'key_size' in the form values and, if it exists, uses the value to validate the key size.
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Active » Needs review
StatusFileSize
new remove_aes_encryption-2664912-3.patch2.65 KB

Here's the patch to remove the AES Encryption plugin from Key.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Assigned: rlhawk » Unassigned
nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented
Assigned: Unassigned » nerdstein

Assigning to myself to review.

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented

All automated tests passed.

Smoke test failed due to no data cleanup. 

Uncaught PHP Exception Drupal\\Component\\Plugin\\Exception\\PluginNotFoundException: "The "aes_encryption" plugin does not exist."

I had an existing Key with type aes_encryption.

I'm wondering if we should add a cleanup routine to delete existing keys that match.

Thoughts?

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
commented

When reversing the patch, I was able to delete the config entity with type aes_encryption. I reloaded the patch and smoke tested, looks good.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented

An update hook could be added in order to remove keys that have key type "aes_encryption".
Be aware, however, of the scenario when this update hook gets called after the Encrypt-module version of key type "aes_encryption" (see https://github.com/d8-contrib-modules/encrypt/pull/82) would already be enabled. In that case, it would remove valid keys.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented

Perhaps a clear warning in the release notes for the next version of Key 8.x could be sufficient, given the (currently) low number of implementations?

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented

I think that's a good plan.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented
Status: Needs review » Reviewed & tested by the community

Seems like we can get this in then?
The same keytype is already defined in the current D8 version of Encrypt BTW.

  • rlhawk committed 5ec2c5a on 8.x-1.x 
    Issue #2664912 by rlhawk, nerdstein, svendecabooter: Remove AES...
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.