Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Laravel 5.5 - data is stored as decrypted after model update #22

Open
bjauy opened this issue Sep 15, 2017 · 16 comments
Open

Laravel 5.5 - data is stored as decrypted after model update #22

bjauy opened this issue Sep 15, 2017 · 16 comments

Comments

@bjauy
Copy link

bjauy commented Sep 15, 2017

I have been testing this package for few days on Laravel 5.5. Recently, I've checked table content and some of the rows weren't encrypted at all.

After few tests in artisan tinker I've nailed the issue to updates that make the data unencrypted. Model creation works as expected.

The problem happens also in default L5.5 installation, using MySQL (MariaDB 10.1.26 exactly):

  1. Database and package setup:
composer create-project --prefer-dist laravel/laravel laravel55
cd laravel55
composer require delatbabel/elocryptfive
php artisan make:auth
php artisan migrate:fresh

(if there is an error with key length, change encoding values in config/database.php to 'utf8' and 'utf8_general_ci' accordingly)

  1. Add below to app/User.php:
use Delatbabel\Elocrypt\Elocrypt;

class User extends Authenticatable
{
    use Elocrypt;

   [...]

   protected $encrypts = [ 'name', ];

   [...]
  • add provider to ```config/app.php`
  1. Test in tinker:
>>> User::create(['name' => 'test', 'email' => 'test@example.com', 'password' => Hash::make('test')]);

The value for name is encrypted.

>>> User::find(1)->update(['email' => 'example@example.com']);

After the update table preview shows data as unecrypted.

@bjauy bjauy changed the title Laravel 5.5 - data is stored as decrypted after update Laravel 5.5 - data is stored as decrypted after model update Sep 15, 2017
@dokicro
Copy link

dokicro commented Sep 20, 2017

Hey, I have the same problem did you found any solutions?

@bjauy
Copy link
Author

bjauy commented Sep 21, 2017

Yes, you need to override getDirty() method in your models which depend on Elocrypt trait:

public function getDirty()
{
    $dirty = [];

    foreach ($this->attributes as $key => $value) {
        if (! $this->originalIsEquivalent($key, $value)) {
                    $dirty[$key] = $value;
        }
    }

    return $dirty;
}

@dokicro
Copy link

dokicro commented Sep 23, 2017

Thanks 👍

@delatbabel
Copy link
Owner

Hrm, interesting. I wonder if it can be over-ridden in the trait and if doing so will cause breakages on prior versions of Laravel? I haven't started migrating any projects to L5.5 yet although it's a planned change with the new LTS release. I will keep this ticket open for monitoring and if anyone has any further information.

@bjauy
Copy link
Author

bjauy commented Sep 27, 2017

IIRC originalIsNumericallyEquivalent method was renamed (and refactored) to originalIsEquivalent in L5.5, so simply adding getDirty would break the trait for users of older versions.

Maybe we could just add second, optional trait with just getDirty() overriden? This, and an information in the readme should be enough IMHO.

Users of older Laravel versions wouldn't be affected by the changes.

I can prepare pull request if you don't mind and if the change is ok for you.

@schonhose
Copy link

I just want to chime in that I am also looking to migrate from Laravel 5.1 to 5.5. I use this package to encrypt data in my database so I welcome any ideas on how to make it work on 5.5.

@mrgrain
Copy link

mrgrain commented Nov 3, 2017

There is also an other bug with Laravel 5.5: If an attribute is casted, the getDirty will fail as well in certain cases. Essentially Laravel is casting the encrypted values which will result in different encrypted values being regarded as the same. E.g. object, array or json will be tried to be read from JSON. However, since the provided input is encrypted, the casted value evaluates to false no matter what.

Our solution is to overwrite the castAttribute method

    /**
     * Decrypt encrypted data before it is processed by cast attribute
     * @param $key
     * @param $value
     *
     * @return mixed
     */
    protected function castAttribute($key, $value)
    {
        return parent::castAttribute($key, $this->doDecryptAttribute($key, $value));
    }

@delatbabel
Copy link
Owner

Thanks for the information. I'm continuing to track this. I'm about to migrate one project from 5.1 LTS to 5.5 LTS so I will try to build something that works for both LTS versions.

@mrgrain
Copy link

mrgrain commented Dec 3, 2017

Instead of trying to fix it for both versions at the same time (or forks or whatever), @delatbabel you could just release a new version which requires Laravel 5.5

@mbardelmeijer
Copy link

@delatbabel any update on this?

@austinheap
Copy link

I ended up 1) forking this, 2) fixing all the Laravel 5.5 issues, 3) dropping support for Laravel below 5.5, and 4) a bunch of random cleanup & unit tests. Available below:

@ITwrx
Copy link

ITwrx commented Feb 20, 2018

@mrgrain where should one override the castAttribute method?
thanks

@mrgrain
Copy link

mrgrain commented Feb 21, 2018

@ITwrx Overwrite it in the model that needs to be encrypted. We have a base model for all encrypted models which uses Elocrypt's trait and overrides the castAttribute method (amongst other things).

Or you could simply create your own EncryptedModel trait which extends the original Elocrypt one.

@ITwrx
Copy link

ITwrx commented Feb 21, 2018

@mrgrain cool, thanks.

@delatbabel
Copy link
Owner

Hi all, firstly sorry about the delay. I have had a long period of illness, just getting back to health now. I'll certainly look at integrating the patches into a Laravel 5.5 branch shortly.

@ITwrx
Copy link

ITwrx commented Feb 21, 2018

glad you're back. stay well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants