Added support to encrypt and decrypt DateTime, JSON and array data types #24
Conversation
|
@cs-akash-jarad I do not just want to merge this one, it is a little bigger than the others I did. Please review and merge if you are happy. |
|
sure will check. i ll be able to review and merge this weekend. |
|
@cs-akash-jarad any chance that you will be able to get to this soon? |
There was a problem hiding this comment.
I like the idea to support other types than just strings.
What I would change though, is leveraging \Doctrine\DBAL\Types\Type::getType() and it's conversions from and to SQL-representation (which usually are strings) so that we can support all types of already known doctrine-types, including custom ones defined by our users.
This way we can avoid doing the conversion from PHP to string and back ourselves.
The only thing we would need to do, is choose an appropriate Platform to use for our conversions.
An demo-implementation can be found somewhere in my code-review-comments.
I've also noted some places where I'd do some stricter compares.
| if ($encryptType == 'datetime') | ||
| { | ||
| $usedValue = $value->format (self::DATETIME_FORMAT); | ||
| } | ||
| elseif ($encryptType == 'json') | ||
| { | ||
| $usedValue = json_encode($value); | ||
| } | ||
| elseif ($encryptType == 'array') | ||
| { | ||
| $usedValue = serialize($value); | ||
| } |
There was a problem hiding this comment.
I'd use strict-compare here:
| if ($encryptType == 'datetime') | |
| { | |
| $usedValue = $value->format (self::DATETIME_FORMAT); | |
| } | |
| elseif ($encryptType == 'json') | |
| { | |
| $usedValue = json_encode($value); | |
| } | |
| elseif ($encryptType == 'array') | |
| { | |
| $usedValue = serialize($value); | |
| } | |
| if ($encryptType === 'datetime') | |
| { | |
| $usedValue = $value->format (self::DATETIME_FORMAT); | |
| } | |
| elseif ($encryptType === 'json') | |
| { | |
| $usedValue = json_encode($value); | |
| } | |
| elseif ($encryptType === 'array') | |
| { | |
| $usedValue = serialize($value); | |
| } |
| if ($encryptType == 'string') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), $currentPropValue); | ||
| } | ||
| elseif ($encryptType == 'datetime') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), DateTime::createFromFormat(self::DATETIME_FORMAT, $currentPropValue)); | ||
| } | ||
| elseif ($encryptType == 'json') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), json_decode($currentPropValue, true)); | ||
| } | ||
| elseif ($encryptType == 'array') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), unserialize($currentPropValue)); | ||
| } |
There was a problem hiding this comment.
Same here, using strict compare:
| if ($encryptType == 'string') | |
| { | |
| $this->pac->setValue($entity, $refProperty->getName(), $currentPropValue); | |
| } | |
| elseif ($encryptType == 'datetime') | |
| { | |
| $this->pac->setValue($entity, $refProperty->getName(), DateTime::createFromFormat(self::DATETIME_FORMAT, $currentPropValue)); | |
| } | |
| elseif ($encryptType == 'json') | |
| { | |
| $this->pac->setValue($entity, $refProperty->getName(), json_decode($currentPropValue, true)); | |
| } | |
| elseif ($encryptType == 'array') | |
| { | |
| $this->pac->setValue($entity, $refProperty->getName(), unserialize($currentPropValue)); | |
| } | |
| if ($encryptType === 'string') | |
| { | |
| $this->pac->setValue($entity, $refProperty->getName(), $currentPropValue); | |
| } | |
| elseif ($encryptType === 'datetime') | |
| { | |
| $this->pac->setValue($entity, $refProperty->getName(), DateTime::createFromFormat(self::DATETIME_FORMAT, $currentPropValue)); | |
| } | |
| elseif ($encryptType === 'json') | |
| { | |
| $this->pac->setValue($entity, $refProperty->getName(), json_decode($currentPropValue, true)); | |
| } | |
| elseif ($encryptType === 'array') | |
| { | |
| $this->pac->setValue($entity, $refProperty->getName(), unserialize($currentPropValue)); | |
| } |
src/Configuration/Encrypted.php
Outdated
|
|
||
|
|
||
|
|
||
| public function __construct(?string $type = 'string') |
There was a problem hiding this comment.
I would not make this argument nullable. You can now instantiate this class with null, which will immediately throw an InvalidArgumentException.
Making it non-nullable avoids this problem.
We could even help IDEs by adding a docblock to signal only to allow certain values:
/**
* @param 'string'|'datetime'|'json'|'array' $type
*/
public function __construct(?string $type = 'string')
PHPStan is at least able to deduct the possible values: https://phpstan.org/r/c54a63fb-8a9d-41ba-9a6c-655a39e7711a
src/Configuration/Encrypted.php
Outdated
|
|
||
| public function __construct(?string $type = 'string') | ||
| { | ||
| if (!in_array ($type, self::ALLOWED_TYPES)) |
There was a problem hiding this comment.
I'd do a strict test:
| if (!in_array ($type, self::ALLOWED_TYPES)) | |
| if (!in_array($type, self::ALLOWED_TYPES, true)) |
| } | ||
| elseif ($encryptType == 'array') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), unserialize($currentPropValue)); |
There was a problem hiding this comment.
I would limit this as to not unserialize any classes by setting the allowed_classes-option to an empty array:
| $this->pac->setValue($entity, $refProperty->getName(), unserialize($currentPropValue)); | |
| $this->pac->setValue($entity, $refProperty->getName(), unserialize($currentPropValue,['allowed_classes' => []])); |
This to avoid any problems relating to serializing and unserialing classes.
If we need to support that, I would add allowedClasses as an extra parameter to the Encrypted-attribute and limit it to that.
| } | ||
| elseif ($encryptType == 'datetime') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), DateTime::createFromFormat(self::DATETIME_FORMAT, $currentPropValue)); |
There was a problem hiding this comment.
Using this method of storing date-times, does lose the timezone information a bit. I added an extra unittest locally to verify this:
abstract class AbstractDoctrineEncryptSubscriberTestCase extends AbstractFunctionalTestCase
{
public function testEntityWithDateTimePropertiesIsTimezoneResistant()
{
date_default_timezone_set('America/New_York');
$user = new DateTimeJsonArrayTarget();
// Use fixed time to avoid flaky tests based on current time
$datetime = new DateTime('2022-06-01 12:00:00', new \DateTimeZone('Europe/Brussels'));
$user->setDate(clone $datetime);
$this->entityManager->persist($user);
$this->entityManager->flush();
// Clear all data from the entity-manager
$this->entityManager->clear();
// Set default timezone to something else
date_default_timezone_set('Asia/Bangkok');
// Refetch the user-entity
$user = $this->entityManager->find(DateTimeJsonArrayTarget::class, $user->getId());
$entityDate = $user->getDate();
$this->assertEquals($datetime, $entityDate);
$this->assertNotFalse($entityDate->getTimezone());
$this->assertEquals('Europe/Brussels',$entityDate->getTimezone()->getName());
$this->assertEquals($datetime->getTimestamp(), $entityDate->getTimestamp());
}
}Possible solution
Perhaps we can leverage \Doctrine\DBAL\Types\... to do the conversion for us?
I believe all types covert from and to string-values, which would allow us to support basically all types known to doctrine, even the custom ones defined by users of this library.
That way we also do not need to worry about different types of conversion ourselves.
Futher investigation
After looking into the default doctrine-types, the default doctrine-types are also not timezone-aware and will also lose timezone-information.
| $encryptType = $this->getEncryptedPropertyType($refProperty); | ||
| if ($encryptType) { | ||
| $rootEntityName = $entityManager->getClassMetadata(get_class($entity))->rootEntityName; | ||
|
|
||
| $pac = PropertyAccess::createPropertyAccessor(); | ||
| $value = $pac->getValue($entity, $refProperty->getName()); | ||
| if ($encryptorMethod === 'decrypt') { | ||
| if (!is_null($value) and !empty($value)) { | ||
| $value = $this->pac->getValue($entity, $refProperty->getName()); | ||
| if (!is_null($value) and !empty($value)) { | ||
| if ($isEncryptOperation) { | ||
| // Default string value | ||
| $usedValue = $value; | ||
| if ($encryptType == 'datetime') | ||
| { | ||
| $usedValue = $value->format (self::DATETIME_FORMAT); | ||
| } | ||
| elseif ($encryptType == 'json') | ||
| { | ||
| $usedValue = json_encode($value); | ||
| } | ||
| elseif ($encryptType == 'array') | ||
| { | ||
| $usedValue = serialize($value); | ||
| } | ||
|
|
||
| if (isset($this->cachedDecryptions[$rootEntityName][spl_object_id($entity)][$refProperty->getName()][$usedValue])) { | ||
| $this->pac->setValue($entity, $refProperty->getName(), $this->cachedDecryptions[$rootEntityName][spl_object_id($entity)][$refProperty->getName()][$usedValue]); | ||
| } elseif (substr($usedValue, -strlen(self::ENCRYPTION_MARKER)) != self::ENCRYPTION_MARKER) { | ||
| $this->encryptCounter++; | ||
| $currentPropValue = $this->encryptor->encrypt($usedValue).self::ENCRYPTION_MARKER; | ||
| $this->pac->setValue($entity, $refProperty->getName(), $currentPropValue); | ||
| } | ||
| } else { | ||
| if (substr($value, -strlen(self::ENCRYPTION_MARKER)) == self::ENCRYPTION_MARKER) { | ||
| $this->decryptCounter++; | ||
| $currentPropValue = $this->encryptor->decrypt(substr($value, 0, -5)); | ||
| $pac->setValue($entity, $refProperty->getName(), $currentPropValue); | ||
| $this->cachedDecryptions[$rootEntityName][spl_object_id($entity)][$refProperty->getName()][$currentPropValue] = $value; | ||
| } | ||
| } | ||
| } else { | ||
| if (!is_null($value) and !empty($value)) { | ||
| if (isset($this->cachedDecryptions[$rootEntityName][spl_object_id($entity)][$refProperty->getName()][$value])) { | ||
| $pac->setValue($entity, $refProperty->getName(), $this->cachedDecryptions[$rootEntityName][spl_object_id($entity)][$refProperty->getName()][$value]); | ||
| } elseif (substr($value, -strlen(self::ENCRYPTION_MARKER)) != self::ENCRYPTION_MARKER) { | ||
| $this->encryptCounter++; | ||
| $currentPropValue = $this->encryptor->encrypt($value).self::ENCRYPTION_MARKER; | ||
| $pac->setValue($entity, $refProperty->getName(), $currentPropValue); | ||
| if ($encryptType == 'string') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), $currentPropValue); | ||
| } | ||
| elseif ($encryptType == 'datetime') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), DateTime::createFromFormat(self::DATETIME_FORMAT, $currentPropValue)); | ||
| } | ||
| elseif ($encryptType == 'json') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), json_decode($currentPropValue, true)); | ||
| } | ||
| elseif ($encryptType == 'array') | ||
| { | ||
| $this->pac->setValue($entity, $refProperty->getName(), unserialize($currentPropValue)); | ||
| } | ||
| } |
There was a problem hiding this comment.
As a proof of concept, I tried leveraging the doctrine-type-conversion and it worked! Perhaps this could be a better solution than converting the data ourselves:
$encryptDbalType = \Doctrine\DBAL\Types\Type::getType($encryptType);
$platform = new MySQL80Platform();
$rootEntityName = $entityManager->getClassMetadata(get_class($entity))->rootEntityName;
$value = $this->pac->getValue($entity, $refProperty->getName());
if (!is_null($value) and !empty($value)) {
if ($isEncryptOperation) {
// Convert to a string using doctrine-type
$usedValue = $encryptDbalType->convertToDatabaseValue($value, $platform);
if (isset($this->cachedDecryptions[$rootEntityName][spl_object_id($entity)][$refProperty->getName()][$usedValue])) {
$this->pac->setValue($entity, $refProperty->getName(), $this->cachedDecryptions[$rootEntityName][spl_object_id($entity)][$refProperty->getName()][$usedValue]);
} elseif (substr($usedValue, -strlen(self::ENCRYPTION_MARKER)) != self::ENCRYPTION_MARKER) {
$this->encryptCounter++;
$currentPropValue = $this->encryptor->encrypt($usedValue).self::ENCRYPTION_MARKER;
$this->pac->setValue($entity, $refProperty->getName(), $currentPropValue);
}
} else {
if (substr($value, -strlen(self::ENCRYPTION_MARKER)) == self::ENCRYPTION_MARKER) {
$this->decryptCounter++;
$currentPropValue = $this->encryptor->decrypt(substr($value, 0, -5));
$this->cachedDecryptions[$rootEntityName][spl_object_id($entity)][$refProperty->getName()][$currentPropValue] = $value;
// Convert from a string to the PHP-type again using dbal-type
$actualValue = $encryptDbalType->convertToPHPValue($currentPropValue, $platform);
$this->pac->setValue($entity, $refProperty->getName(),$actualValue);
}
}
}| * @Target("PROPERTY") | ||
| */ | ||
| #[Attribute(Attribute::TARGET_PROPERTY)] | ||
| class Encrypted implements Annotation | ||
| final class Encrypted implements Annotation |
There was a problem hiding this comment.
Making this final could theoretically break applications using our library. I don't mind making this final though, as we would expect this to not be extended.
|
@Zombaya I implemented your suggestion of using \Doctrine\DBAL\Types\Type::getType() instead of doing the conversions directly. Note that I used the entity manager getConnection and the connection getDatabasePlatform instead of always just using the MySql Platform. If you approve, then we could merge and finish your pull request to add support for Symfony 7 #28 and the non-breaking changes in #27. Once we have all those changes done, I want to release version 5.3.2 and create 2 branches 5.x and 6.x. In the 6.x branch I would like to start implementing all the breaking changes. I do have an idea I would like to try that I think could possibly make the bundle a little faster and use less memory, but will need to get there to try it out and confirm. |
|
Firstly, sorry to go so long without responding. We had a very hectic period before Christmas and after the holidays and I had little energy left after work to work on open source. The code looks good to me. I would however rethink fetching the platform from the connection. I feel it would be better to use a fixed platform so that:
But code-wise, everything looks good to me. |
@Zombaya have invited you again. Thanks for the willingness to assist and keep the bundle alive. I see your point about the possible data corruption between database platforms, but will that not possibly be an issue as well if we force the use of a mismatching platform? |
|
I don't think the will be a problem of mismatching platform since from doctrine's point of view, we are storing a string. The process is:
After which doctrine decides on how to store that finally encrypted string in the DB. |
…decypt is always the same
No description provided.