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

[9.x] Add support to detect dirty encrypted model attributes #42888

Merged
merged 3 commits into from
Jun 22, 2022
Merged

[9.x] Add support to detect dirty encrypted model attributes #42888

merged 3 commits into from
Jun 22, 2022

Conversation

PhiloNL
Copy link
Contributor

@PhiloNL PhiloNL commented Jun 20, 2022

Currently, a model always determines that an encrypted attribute is modified when you fill a model, even if the value that is being encrypted is the same. This is because it compares the encrypted string, which changes every time you encrypt something regardless of whether the encrypted value is the same.

This PR will add support to detect dirty encrypted model attributes.

$model = Model::create(['billing_address' => ['street' => 'foo']);

$model->fill(['billing_address' => ['street' => 'foo']);
$model->originalIsEquivalent('billing_address'); // Returns false while it must return true

I wasn't sure if the tests should be part of EloquentModelEncryptedCastingTest or DatabaseEloquentModelTest. If they need to be moved, let me know 🙌🏻

This follows up on #38869 and the question from Taylor #38869 (comment).

@PhiloNL
Copy link
Contributor Author

PhiloNL commented Jun 21, 2022

Point of discussion:

public function set($model, $key, $value, $attributes)
{
if (! is_null($value)) {
return [$key => Crypt::encryptString(json_encode($value))];
}
return null;
}

All though this PR fixes the $model->originalIsEquivalent(), $model->isDirty() behavior, calling $model->update() will still update the database with a new encrypted string even though the contents is the same. Preferrably this update is skipped because it's unnecessary.

A possible solution is to add a check to theset() method on the caster and only encrypt if the content is different:

public function set($model, $key, $value, $attributes)
{
    if (! is_null($value)) {
        if(json_encode($model->getOriginal($key)) === json_encode($value)) {
            return [$key => $model->getRawOriginal($key)];
        }

        return [$key => Crypt::encryptString(json_encode($value))];
    }

    return null;
}

@taylorotwell taylorotwell merged commit 63e22b0 into laravel:9.x Jun 22, 2022
@PhiloNL PhiloNL deleted the dirty-encrypted-attributes branch June 22, 2022 21:11
turanjanin pushed a commit to turanjanin/laravel-framework that referenced this pull request Jun 23, 2022
…#42888)

* Add encypted casts

* Style CI changes

* Bugfix “The payload is invalid.”

Only compare if the original is not null
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

Successfully merging this pull request may close these issues.

2 participants